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

IDownstreamApi overloads that take JsonTypeInfo<T> as a parameter to enable source generated Json deserialization for NativeAOT #131

Merged
merged 16 commits into from
Jul 29, 2024

Conversation

dualtagh
Copy link
Contributor

@dualtagh dualtagh commented Jul 19, 2024

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.

Copy link
Contributor

@jmprieur jmprieur left a 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

Directory.Build.props Outdated Show resolved Hide resolved
/// return _downstreamWebApi.CallWebApiForUserAsync&lt;MyItem, MyItem&gt;(
/// ServiceName,
/// nyItem,
/// options =>
Copy link
Contributor

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

@dualtagh dualtagh marked this pull request as ready for review July 23, 2024 16:03
@dualtagh dualtagh requested a review from a team as a code owner July 23, 2024 16:03
Copy link
Contributor

@jmprieur jmprieur left a 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?

Directory.Build.props Outdated Show resolved Hide resolved
Copy link
Collaborator

@jennyf19 jennyf19 left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

LGTM. Nice work.

@dualtagh dualtagh merged commit 860d228 into main Jul 29, 2024
4 checks passed
@dualtagh dualtagh deleted the dualtagh/json_overloads_for_aot branch July 29, 2024 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants