feat: move to System.Text.Json as serialiser #249
Conversation
| using (var request = new HttpRequestMessage(HttpMethod.Post, requestUri)) | ||
| { | ||
| request.Content = new StreamContent(memoryStream, bufferSize); | ||
| request.Content = new StringContent(JsonSerializer.Serialize(registration, options), Encoding.UTF8, "application/json"); |
There was a problem hiding this comment.
Swap out new streaming. This should have a default buffer of 1024 * 4 bytes anyway
| @@ -71,6 +71,7 @@ | |||
| </PackageReference> | |||
| <PackageReference Include="murmurhash" Version="1.0.3" /> | |||
| <PackageReference Include="NuGet.Versioning" Version="6.1.0" /> | |||
| <PackageReference Include="System.Text.Json" Version="8.0.5" /> | |||
There was a problem hiding this comment.
Force a reference to System.Text.Json for older versions that don't have it built in
| @@ -8,7 +8,6 @@ | |||
| using Unleash.Internal; | |||
| using Unleash.Events; | |||
| using Unleash.Communication; | |||
There was a problem hiding this comment.
Seems to be dead code. It got yeeted
|
Closes #221 |
There was a problem hiding this comment.
This is great! You have my blessing, feels good to get some of this complexity removed. A little paranoid testing as we've already discussed, for instance before and after this change with backups created by one and then loaded by the other might not be a bad idea.
|
@daveleek I'm less worried about loading features from file, that's effectively a cache miss on major upgrade which feels okay. Bit worried about Edge so agree we need to validate this more before release We can play tomorrow and try break it! |
This removes the deprecated option to set your own JSON serializer. This should be very low impact on consumers, since this just requires them to remove a custom serializer if one had previously be set. The SDK now uses
System.Text.Jsonwhich is built into most modern versions of .NET and is added as a dependency for legacy versions that don't support it natively.Almost all of this PR is noise removing references, I'll mark relevant sections with some comments
Discussion
There's some, frankly, weird stuff around streams in the SDK at the moment. I'm 99% sure that's an artifact of the old, slightly awkward interfaces. I've chosen to throw that out, write directly and let the HTTP client deal with the buffering. I'm pretty sure that's not a big loss since the old implementation was flushing to the streams multiple times and the new implementation can use async to not have to worry too much about what's being blocked