Replace JsonSerializer with DataContractJsonSerializer in Microsoft.Toolkit.Uwp. #3637

Rosuavio · 2020-12-17T22:56:22Z

Fixes #3636

Change implementation of JsonObjectSerializer to use DataContractJsonSerializer

PR Type

What kind of change does this PR introduce?

What is the current behavior?

Microsoft.Toolkit.Uwp uses JsonSerializer

What is the new behavior?

Microsoft.Toolkit.Uwp uses DataContractJsonSerializer

PR Checklist

Please check if your PR fulfills the following requirements:

Tested code with current supported SDKs
Pull Request has been submitted to the documentation repository instructions. 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
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

Other information

ghost · 2020-12-17T22:56:26Z

Thanks RosarioPulella 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 🙌

michael-hawker · 2020-12-17T23:12:03Z

Awesome, that seems pretty straight-forward. Have you tested the update scenario for SystemInformation yet where if an app was initialized with a Newtonsoft serializer and then switches to this one all the data loads and is re-saved properly? I think that's the biggest thing for us to validate.

Outside of that it's just release notes and documentation. Adding the 'breaking changes' label here for tracking when we write the release notes as this could cause changes for developers that are doing more complex scenarios (but we have mitigation).

FYI @vgromfeld @simop-msft

michael-hawker · 2020-12-18T20:26:21Z

@RosarioPulella CI failed - looks like some tests need updating? (Or maybe a sign that we broke something?)

Rosuavio · 2020-12-23T21:56:24Z

Have you tested the update scenario for SystemInformation yet where if an app was initialized with a Newtonsoft serializer and then switches to this one all the data loads and is re-saved properly? I think that's the biggest thing for us to validate.

I am not sure how to initialize an app using the old version of the lib in a unit test with the new lib also included in the same proj. I am looking into including the old package Microsoft.Toolkit.Uwp ver 6.1.1 using something like this maybe https://stackoverflow.com/questions/42715564/using-2-different-versions-of-the-same-dll/49238410?

Rosuavio · 2020-12-23T21:59:30Z

It seems like I would have to build and run an app with 6.1.1 then build and run with the local project ref to Microsoft.Toolkit.Uwp inside the unit test... Looking into it.

vgromfeld · 2021-01-04T08:52:02Z

Is there any impact on the final binary size ?
It shouldn't if the SystemInformation class is not used.

michael-hawker · 2021-01-04T19:49:30Z

Is there any impact on the final binary size ?
It shouldn't if the SystemInformation class is not used.

@vgromfeld we were still seeing the dlls included in the SmokeTest apps and report?

Added legacy public class read test. Add public class for testing.

michael-hawker

Definitely want to ensure we document these changes. But I think as we said as long we've tested SystemInformation we can always guide developers to use the previous Newtonsoft or System.Text.Json for compatibility.

ghost · 2021-01-05T17:58:04Z

Hello @michael-hawker!

Because this pull request has the auto merge :zap: label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

Microsoft.Toolkit.Uwp/Helpers/ObjectStorage/BaseObjectStorageHelper.cs

Microsoft.Toolkit.Uwp/Helpers/ObjectStorage/LocalObjectStorageHelper.cs

Microsoft.Toolkit.Uwp/Helpers/ObjectStorage/RoamingObjectStorageHelper.cs

…elper.cs Co-authored-by: Michael Hawker MSFT (XAML Llama) <[email protected]>

…Helper.cs Co-authored-by: Michael Hawker MSFT (XAML Llama) <[email protected]>

…geHelper.cs Co-authored-by: Michael Hawker MSFT (XAML Llama) <[email protected]>

michael-hawker · 2021-01-06T00:05:16Z

@msftbot merge when @vgromfeld approves

ghost · 2021-01-06T00:05:20Z

Hello @michael-hawker!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I'll only merge this pull request if it's approved by @vgromfeld

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

azchohfi · 2021-01-06T00:13:53Z

Microsoft.Toolkit.Uwp.UI.Controls/Microsoft.Toolkit.Uwp.UI.Controls.csproj

@@ -47,6 +47,7 @@

  <ItemGroup>
    <PackageReference Include="System.ValueTuple" Version="4.5.0" />
+	<PackageReference Include="System.Text.Json" Version="4.7.2" />


Suggested change

<PackageReference Include="System.Text.Json" Version="4.7.2" />

<PackageReference Include="System.Text.Json" Version="4.7.2" />

Fallow up to #3637 Applying #3637 (comment)

@Sergio0694

## 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?   - Feature  - Refactoring (no functional changes, no api changes)     ## 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:  - [ ] 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  ## 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) - [ ] ???

ghost requested review from michael-hawker, azchohfi and Kyaa-dost December 17, 2020 22:56

ghost added the improvements ✨ label Dec 17, 2020

michael-hawker added the introduce breaking changes 💥 label Dec 17, 2020

michael-hawker added this to the 7.0 milestone Dec 17, 2020

ghost added the in progress 🚧 label Dec 17, 2020

Rosuavio added 3 commits January 4, 2021 15:01

Replace JsonSerializer with DataContractJsonSerializer

d6b3ab1

Ignore failling legacy test

f339134

Added legacy public class read test. Add public class for testing.

Ignore failling legacy test.

31e1a0f

michael-hawker approved these changes Jan 5, 2021

View reviewed changes

michael-hawker added the auto merge ⚡ label Jan 5, 2021

Rosuavio added 3 commits January 5, 2021 15:40

Document change in default ctor behavior for sotrage helpers

7a7fc5b

Missing using for docs

6553db6

Remove unneeded refs to System.Text.Json

df7497c

michael-hawker reviewed Jan 5, 2021

View reviewed changes

Rosuavio and others added 3 commits January 5, 2021 16:53

Update Microsoft.Toolkit.Uwp/Helpers/ObjectStorage/BaseObjectStorageH…

e05263c

…elper.cs Co-authored-by: Michael Hawker MSFT (XAML Llama) <[email protected]>

Update Microsoft.Toolkit.Uwp/Helpers/ObjectStorage/LocalObjectStorage…

2ed00ab

…Helper.cs Co-authored-by: Michael Hawker MSFT (XAML Llama) <[email protected]>

Update Microsoft.Toolkit.Uwp/Helpers/ObjectStorage/RoamingObjectStora…

bf475e9

…geHelper.cs Co-authored-by: Michael Hawker MSFT (XAML Llama) <[email protected]>

Rosuavio added for-review 📖 To evaluate and validate the Issues or PR reviewers wanted ✋ and removed in progress 🚧 labels Jan 5, 2021

michael-hawker requested a review from vgromfeld January 6, 2021 00:05

azchohfi reviewed Jan 6, 2021

View reviewed changes

vgromfeld approved these changes Jan 6, 2021

View reviewed changes

ghost merged commit b4f1b2f into CommunityToolkit:master Jan 6, 2021

michael-hawker mentioned this pull request Jan 6, 2021

[Build/CI] Smoke Tests don't run from current build #3657

Closed

Rosuavio mentioned this pull request Jan 6, 2021

Fix spacing in csproj #3659

Merged

Rosuavio deleted the remove-textjson branch January 6, 2021 16:34

ghost pushed a commit that referenced this pull request Jan 6, 2021

Fix spacing in csproj (#3659)

05a42e3

Fallow up to #3637 Applying #3637 (comment)

This was referenced Jan 22, 2021

Reduce the toolkit impact on application size #3145

Closed

Investigate Basic Serializer for BaseObjectStorageHelper #3692

Closed

SystemSerializer update for BaseObjectStorageHelper #3702

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Replace JsonSerializer with DataContractJsonSerializer in Microsoft.Toolkit.Uwp. #3637

Replace JsonSerializer with DataContractJsonSerializer in Microsoft.Toolkit.Uwp. #3637

Uh oh!

Rosuavio commented Dec 17, 2020

Uh oh!

ghost commented Dec 17, 2020

Uh oh!

michael-hawker commented Dec 17, 2020

Uh oh!

michael-hawker commented Dec 18, 2020

Uh oh!

Rosuavio commented Dec 23, 2020

Uh oh!

Rosuavio commented Dec 23, 2020 •

edited

Loading

Uh oh!

vgromfeld commented Jan 4, 2021

Uh oh!

michael-hawker commented Jan 4, 2021

Uh oh!

michael-hawker left a comment

Uh oh!

ghost commented Jan 5, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

michael-hawker commented Jan 6, 2021

Uh oh!

ghost commented Jan 6, 2021

Uh oh!

azchohfi Jan 6, 2021

Uh oh!

Uh oh!

	<PackageReference Include="System.Text.Json" Version="4.7.2" />
	<PackageReference Include="System.Text.Json" Version="4.7.2" />

Replace JsonSerializer with DataContractJsonSerializer in Microsoft.Toolkit.Uwp. #3637

Replace JsonSerializer with DataContractJsonSerializer in Microsoft.Toolkit.Uwp. #3637

Uh oh!

Conversation

Rosuavio commented Dec 17, 2020

Fixes #3636

PR Type

What is the current behavior?

What is the new behavior?

PR Checklist

Other information

Uh oh!

ghost commented Dec 17, 2020

Uh oh!

michael-hawker commented Dec 17, 2020

Uh oh!

michael-hawker commented Dec 18, 2020

Uh oh!

Rosuavio commented Dec 23, 2020

Uh oh!

Rosuavio commented Dec 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vgromfeld commented Jan 4, 2021

Uh oh!

michael-hawker commented Jan 4, 2021

Uh oh!

michael-hawker left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Jan 5, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

michael-hawker commented Jan 6, 2021

Uh oh!

ghost commented Jan 6, 2021

Uh oh!

azchohfi Jan 6, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Rosuavio commented Dec 23, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.