-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace Newtonsoft.Json with the new .Net JsonSerializer. #3414
Conversation
Thanks azchohfi for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌 |
Since $type in JSON is not recommended, maybe its time we bump the version of the json to version 2, and create a new format that supports this better. @michael-hawker ? We could also support the same file version and just create a custom converter. |
I fixed it with a few custom json converters. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, glad to see the toolkit switching to System.Text.Json
🚀🚀🚀
Just left a couple suggestions and questions, but overall I like this a lot!
Microsoft.Toolkit.Services/Services/LinkedIn/LinkedInDataProvider.cs
Outdated
Show resolved
Hide resolved
Microsoft.Toolkit.Services/Services/LinkedIn/LinkedInDataProvider.cs
Outdated
Show resolved
Hide resolved
Microsoft.Toolkit.Services/Services/Twitter/TwitterCoordinatesConverter.cs
Outdated
Show resolved
Hide resolved
Microsoft.Toolkit.Services/Services/Twitter/TwitterCoordinatesConverter.cs
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.Services/Properties/Microsoft.Toolkit.Uwp.Services.rd.xml
Outdated
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI.Controls/InfiniteCanvas/Drawables/InkDrawable.cs
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI.Controls/InfiniteCanvas/Drawables/TextDrawable.cs
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI.Controls/InfiniteCanvas/JsonConverters/IDrawableConverter.cs
Outdated
Show resolved
Hide resolved
…in the services project.
This PR has been marked as "needs attention 👋" and awaiting a response from the team. |
…Converter.cs Co-authored-by: Sergio Pedri <[email protected]>
Microsoft.Toolkit.Services/Services/LinkedIn/LinkedInDataProvider.cs
Outdated
Show resolved
Hide resolved
…der.cs Co-authored-by: Sergio Pedri <[email protected]>
This PR has been marked as "needs attention 👋" and awaiting a response from the team. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! 🎉
Co-authored-by: Sergio Pedri <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looks good, but I encountered an issue. An exception was thrown when I tried to "Import and Load" json generated from saving the canvas while on 1b08bd6.
I think we should try to support json generated from old version of the InfanteCanvas
, can we fix this?
Attached is the json, the max view image, and the exception.
Infinite Canvas.zip
Nice catch! I'll look it up today! |
Very simple fix. I'll just create a regression test to keep this stable. |
This PR has been marked as "needs attention 👋" and awaiting a response from the team. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't get it to crash with any old json (no matter how hard I scribble). Good work!
@azchohfi looks like the last bit is that the unit test needs some rd.xml or other fix to load:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving, as I think we're good once the CI is fixed.
@azchohfi I think this means the only thing we're using Json serialization for in the UWP library is the StorageHelper, eh? And then for Controls it's the Infinite Canvas?
I think that is it, yes. |
## Fixes #3692 This introduces a new light-weight serializer which we can use within the toolkit for the `SystemInformation` class still. And then developers with more complex type needs can specify their own serializer for the `BaseObjectStorageHelper` classes. See the related issue for more details. Follow-on to #3637 and #3414 ## PR Type What kind of change does this PR introduce? <!-- Please uncomment one or more that apply to this PR. --> <!-- - Bugfix --> - Feature <!-- - Code style update (formatting) --> - Refactoring (no functional changes, no api changes) <!-- - Build or CI related changes --> <!-- - Documentation content changes --> <!-- - Sample app changes --> <!-- - Other... Please describe: --> ## What is the current behavior? Using `DataContractJsonSerializer`, though we had used `Newtonsoft.Json` in 6.1. ## What is the new behavior? Use new `SystemSerializer` which passes values to/from the Windows `ApplicationDataContainer` APIs directly. Updates our internal usages of this to the new system. Requires developer using the `BaseObjectStorageHelper` to provide a serializer implementation. This of course does provide a 'default'/system information within the toolkit now compared to our original thought of providing none at all before the `SystemInformation` problem was identified. ## PR Checklist Please check if your PR fulfills the following requirements: - [x] Tested code with current [supported SDKs](../readme.md#supported) - [ ] Pull Request has been submitted to the documentation repository [instructions](..\contributing.md#docs). Link: <!-- docs PR link --> - [ ] Sample in sample app has been added / updated (for bug fixes / features) - [ ] Icon has been created (if new sample) following the [Thumbnail Style Guide and templates](https://github.com/windows-toolkit/WindowsCommunityToolkit-design-assets) - [ ] New major technical changes in the toolkit have or will be added to the [Wiki](https://github.com/windows-toolkit/WindowsCommunityToolkit/wiki) e.g. build changes, source generators, testing infrastructure, sample creation changes, etc... - [ ] Tests for the changes have been added (for bug fixes / features) (if applicable) - [ ] Header has been added to all new source files (run *build/UpdateHeaders.bat*) - [ ] Contains **NO** breaking changes <!-- If this PR contains a breaking change, please describe the impact and migration path for existing applications below. Please note that breaking changes are likely to be rejected within minor release cycles or held until major versions. --> ## Other information I tested copying the system.dat file from the Settings folder of our 6.1 sample app to the development version with the changes, no issues with `SystemInformation` reading the old data. Updated/added unit tests to test the old json layer vs. reading from the new SystemSerializer as well as just testing a new `System.Text.Json` complex scenario. Added tests for exceptions. TODO: - [ ] Add more tests? - [x] Run XAML Islands SystemInformation test - [ ] Do we want `SystemSerializer` to handle a few more 'basic' (but non-primitive) types? E.g. @Sergio0694 has a [similar implementation for a different related system](https://github.com/Sergio0694/Brainf_ckSharp/blob/master/src/Brainf_ckSharp.Services.Uwp/SettingsService.cs) - [ ] ???
Fixes #3060, #889, #2957
This completely removes Newtonsoft.Json in favor of the new DotNet JsonSerializer.
We need to test this thoroughly. If something was serialized with Newtonsoft.Json and deserialized with this, it needs to work.
PR Type
What kind of change does this PR introduce?
What is the current behavior?
We rely on Newtonsoft.Json for any Json serialization or deserialization.
What is the new behavior?
We depend on the new System.Text.Json package.
PR Checklist
Please check if your PR fulfills the following requirements: