-
Notifications
You must be signed in to change notification settings - Fork 3
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
IDownstreamApi overloads that take JsonTypeInfo<T> as a parameter to enable source generated Json deserialization for NativeAOT #131
Conversation
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.
LGTM
I added a few suggestions
src/Microsoft.Identity.Abstractions/DownstreamApi/IDownstreamApi.HttpMethods.tt
Show resolved
Hide resolved
src/Microsoft.Identity.Abstractions/DownstreamApi/IDownstreamApi.HttpMethods.tt
Outdated
Show resolved
Hide resolved
/// return _downstreamWebApi.CallWebApiForUserAsync<MyItem, MyItem>( | ||
/// ServiceName, | ||
/// nyItem, | ||
/// options => |
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.
You might want to update this code snippet (which goes in the reference documentation). See the Examples paragraph of https://learn.microsoft.com/en-us/dotnet/api/microsoft.identity.abstractions.idownstreamapi.callapiforuserasync?view=msal-model-dotnet-latest for instance
src/Microsoft.Identity.Abstractions/Microsoft.Identity.Abstractions.csproj
Outdated
Show resolved
Hide resolved
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.
LGTM
Thanks @dualtagh
you might want to run:
dotnet pack /p:GenerateCompatibilitySuppressionFile=true
to generate the API compat suppression files (until we move to version 7.0, at which point you'd remove them?
src/Microsoft.Identity.Abstractions/DownstreamApi/IDownstreamApi.HttpMethods.cs
Outdated
Show resolved
Hide resolved
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.
src/Microsoft.Identity.Abstractions/CompatibilitySuppressions.xml
Outdated
Show resolved
Hide resolved
src/Microsoft.Identity.Abstractions/DownstreamApi/IDownstreamApi.HttpMethods.tt
Show resolved
Hide resolved
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.
LGTM. Nice work.
AzureAD/microsoft-identity-web#2930
Adding overloads to IDownstreamApi that accept JsonTypeInfo for inputs / outputs so that source generated Json serialization can work in a NativeAOT context.