Migrate from Newtonsoft.Json to System.Text.Json #672
Conversation
|
Also, in commit 6a87013 I improved all the HTTP requests to deserialize the JSON payload directly from the HTTP response stream instead of reading the response as string then deserializing to model objects. This is an improvement in both speed and memory allocations. |
| using (var stream = await streamTask) | ||
| using (var reader = new StreamReader(stream, new UTF8Encoding(false))) | ||
| using (var jsonReader = new JsonTextReader(reader) { SupportMultipleContent = true }) | ||
| using (cancellationToken.Register(() => tcs.TrySetCanceled(cancellationToken))) | ||
| { |
There was a problem hiding this comment.
Verify this task can be cancelled by running the test CreateContainerAsync_TimeoutExpires_Fails.
There was a problem hiding this comment.
I removed the Skip reason and I can confirm that the tests with the 3 different timeouts are passing. ✅
0xced
force-pushed
the
System.Text.Json
branch
2 times, most recently
from
bb945b0
to
2ff7b0e
Compare
Why not just use HttpClientJsonExtensions with source generators? I'm missing what the added value is for having generic methods with no constraints for making the HTTP requests. |
| @@ -5,6 +5,7 @@ | |||
| <PropertyGroup> | |||
| <IsPackable>true</IsPackable> | |||
| <TargetFrameworks>netstandard2.0;netstandard2.1</TargetFrameworks> | |||
| <LangVersion>latest</LangVersion> | |||
There was a problem hiding this comment.
Isn't latest the default? What's the added benefit of this change?
There was a problem hiding this comment.
The default language version depends on the target framework, see the C# language versioning documentation.
For .NET Standard 2.0 the language version is C# 7.3 and for .NET Standard 2.1 the language version is C# 8.0.
I don't remember everything I changed but there's a high probability that I used some feature that requires C# greater than 7.3.
There was a problem hiding this comment.
My mistake.
But it may be different for different SDK versions.
But, if you're aiming for a version higher than the supported for that target, you need to be very careful.
I don't remember but it was probably because of the dozen of |
|
It LGTM. For now, while we don't move toward the new code which drop the Go generator, this change to STJ will help others and yes, the extra allocations with strings will be dealt with in a separated PR. For now let's add it. Also, the CI error is unrelated, so I'm good to push it as is. Will fix the CI separately. Thank you @0xced! |
Everything is now truly async, no more fake
Task.Factory.StartNewasync.