Skip to content
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

Initial port to System.Text.Json. #3428

Closed
wants to merge 9 commits into from

Conversation

azchohfi
Copy link

Fixes #3407

Changes proposed in this request
Removes Newtonsoft.Json, and replaces it with System.Text.Json

Testing
All Unit Tests are passing, but there is more work to be done since this is still not using source generators, so trimming would still affect it.

Performance impact
Performance should greatly improve after the source generators are implemented since no reflection will be used anymore for the Json parsing.

@azchohfi azchohfi requested a review from bgavrilMS June 28, 2022 00:24
@bgavrilMS bgavrilMS requested a review from pmaytak June 28, 2022 09:02
@@ -46,7 +46,7 @@ public TraceTelemetryConfig()
/// <remarks>This API is experimental and it may change in future versions of the library without an major version increment</remarks>
public Action<ITelemetryEventPayload> DispatchAction => payload =>
{
var j = new JObject();
var j = new JsonObject();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this class can be hard deprecated if needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's already obsoleted. Can't delete it since it affects public API.

[Obsolete("Telemetry is sent automatically by MSAL.NET. See https://aka.ms/msal-net-telemetry.", false)]
[EditorBrowsable(EditorBrowsableState.Never)]
public class TraceTelemetryConfig : ITelemetryConfig

@azchohfi
Copy link
Author

@pmaytak/@bgavrilMS I've just pushed the changes that added the source generated Json parsing.

@azchohfi
Copy link
Author

There are still some IL trimming warnings, so this can't be merged as-is yet. Still investigating, but it is probably something that is being generated for a property that shouldn't be.

@pmaytak
Copy link
Contributor

pmaytak commented Jun 28, 2022

Thanks @azchohfi. I'll take a deeper look, but initial thoughts:

  • If I remember correctly, some defaults are different for Newtonsoft and System.Text.Json, like case-sensitive vs case-insensitive, need to make sure the behavior is the same. Need to verify if our JSON tests are sufficient.
  • Thinking it might be good to aggregate all JSON related code into one class, like JsonHelper. (Although this will be fine to do in a separate PR).
  • We do have some perf tests that we can run for before/after.

@azchohfi
Copy link
Author

We should also consider adding a .Net6 target for other platforms, such as Linux/MacOS, so we support trimming for them too. The way the project is structured, we only target net5.0-windows10.0.17763.0, so a Linux project would probably not benefit completely from this.

@azchohfi
Copy link
Author

As well as a test app with trimming enabled to make sure things are properly trimmed.

@azchohfi
Copy link
Author

We should also consider adding a .Net6 target for other platforms, such as Linux/MacOS, so we support trimming for them too. The way the project is structured, we only target net5.0-windows10.0.17763.0, so a Linux project would probably not benefit completely from this.

Yeah, this is a must. If we don't, then a simple net6.0 project will reference the netcore app 3.1 TFM, which will have trimming issues since its methods can't be annotated properly.

@azchohfi
Copy link
Author

@Sergio0694 is it possible to still annotate the one method that is complaining about dynamic access on netcoreapp3.1?
It is a simple call to System.Activator.CreateInstance(Type,Object[]), which is mostly only used by the tests, but it is a public method and I don't want to remove a public API with this PR. We could re-implement it with a switch statement since we are talking about 4 possible types that are created here, but I wonder in general if we have a better answer.

@azchohfi
Copy link
Author

Btw, this is the call:

MsalException ex = Activator.CreateInstance(exceptionType, errorCode, errorMessage) as MsalException;

@azchohfi
Copy link
Author

Ok, it is definitely working \o/ got all the issues working with this last commit.

@azchohfi
Copy link
Author

I'll send another PR from a branch of this repo, not from my fork, so it will run the build properly.

@azchohfi
Copy link
Author

Closed in favor of #3435

@azchohfi azchohfi closed this Jun 29, 2022
@azchohfi azchohfi deleted the alzollin/systemTextJson branch June 29, 2022 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants