Skip to content

Annotate WebAssembly.Authentication for nullability #43442

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Nov 14, 2022
Merged

Annotate WebAssembly.Authentication for nullability #43442

merged 3 commits into from
Nov 14, 2022

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Aug 22, 2022

Addresses #5680

@JamesNK JamesNK added the area-blazor Includes: Blazor, Razor Components label Aug 22, 2022
@JamesNK JamesNK requested a review from a team as a code owner August 22, 2022 06:27
JamesNK
JamesNK commented Aug 22, 2022
View reviewed changes
src/Components/WebAssembly/WebAssembly.Authentication/src/NavigationManagerExtensions.cs Outdated
{
manager.NavigateTo(logoutPath, new NavigationOptions
{
HistoryEntryState = new InteractiveRequestOptions
{
Interaction = InteractionType.SignOut,
ReturnUrl = returnUrl
ReturnUrl = returnUrl ?? manager.Uri
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure about this change. Can InteractiveRequestOptions.ReturnUrl be null?

Copy link
Member

Choose a reason for hiding this comment

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

It seems like we assume the return URL can be null elsewhere in WebAssembly.Authentication, e.g. RemoteAuthenticatorViewCore. Maybe we can make InteractiveRequestOptions.ReturnUrl nullable and not required?

cc @javiercn

@JamesNK JamesNK requested a review from a team as a code owner August 22, 2022 07:18
@JamesNK
Copy link
Member Author

JamesNK commented Aug 22, 2022

@dotnet/aspnet-build How do I fix the public API files.

I tried updating just the Unshipped.txt. As an example:

*REMOVED*~Microsoft.Extensions.DependencyInjection.IRemoteAuthenticationBuilder<TRemoteAuthenticationState, TAccount>
*REMOVED*~Microsoft.Extensions.DependencyInjection.IRemoteAuthenticationBuilder<TRemoteAuthenticationState, TAccount>.Services.get -> Microsoft.Extensions.DependencyInjection.IServiceCollection

Microsoft.Extensions.DependencyInjection.IRemoteAuthenticationBuilder<TRemoteAuthenticationState, TAccount>
Microsoft.Extensions.DependencyInjection.IRemoteAuthenticationBuilder<TRemoteAuthenticationState, TAccount>.Services.get -> Microsoft.Extensions.DependencyInjection.IServiceCollection!

But I got warnings that the type exists twice. How do I fix this?

Warning RS0025 The symbol 'Microsoft.Extensions.DependencyInjection.IRemoteAuthenticationBuilder<TRemoteAuthenticationState, TAccount>' appears more than once in the public API files Microsoft.AspNetCore.Components.WebAssembly.Authentication C:\Development\Source\aspnetcore\src\Components\WebAssembly\WebAssembly.Authentication\src\PublicAPI.Unshipped.txt 32 Active

@BrennanConroy
Copy link
Member

Update the shipped.txt file in this case. The analyzer doesn't work well when going from non-nullable to nullable APIs.

@JamesNK JamesNK force-pushed the jamesnk/nullable-leftovers branch from da54e93 to dbb4946 Compare August 23, 2022 05:26
@JamesNK
Copy link
Member Author

JamesNK commented Aug 23, 2022 •
edited
Loading

@dotnet/aspnet-build How do I make the build pass with the shipped.txt changes?

@BrennanConroy
Copy link
Member

How do I make the build pass with the shipped.txt changes?

You don't. Someone with permissions will need to merge.

@JamesNK JamesNK force-pushed the jamesnk/nullable-leftovers branch from dbb4946 to a804000 Compare September 21, 2022 23:44
@JamesNK
Copy link
Member Author

JamesNK commented Sep 21, 2022 •
edited
Loading

@dotnet/aspnet-blazor-eng Please review

@JamesNK
Copy link
Member Author

JamesNK commented Sep 24, 2022

@mkArtakMSFT

@mkArtakMSFT
Copy link
Contributor

Thanks @JamesNK.
@MackinnonBuck can you please review this? Thanks!

MackinnonBuck
MackinnonBuck reviewed Oct 5, 2022
View reviewed changes
Copy link
Member

@MackinnonBuck MackinnonBuck left a comment

Choose a reason for hiding this comment

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

This looks good to me. I think this is the main question that we still need to make a decision on. This can be merged once we resolve that.

@javiercn do you have any thoughts on this?

@JamesNK
Copy link
Member Author

JamesNK commented Oct 17, 2022

I'd like to be able to merge this. Can the SME please review and approve/require changes 🙏

@javiercn
Copy link
Member

@JamesNK let me take a look.

1 similar comment
@javiercn
Copy link
Member

@JamesNK let me take a look.

@JamesNK JamesNK force-pushed the jamesnk/nullable-leftovers branch from a804000 to c46f84d Compare October 18, 2022 05:06
@JamesNK
Copy link
Member Author

JamesNK commented Nov 2, 2022

@javiercn ping

javiercn
javiercn approved these changes Nov 2, 2022
View reviewed changes
Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

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

One small comment, otherwise looks good.

@JamesNK JamesNK force-pushed the jamesnk/nullable-leftovers branch from c46f84d to e4e38d0 Compare November 3, 2022 01:12
@dougbu dougbu merged commit 0579297 into main Nov 14, 2022
@dougbu dougbu deleted the jamesnk/nullable-leftovers branch November 14, 2022 23:56
@ghost ghost added this to the 8.0-preview1 milestone Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MackinnonBuck MackinnonBuck MackinnonBuck left review comments

@javiercn javiercn javiercn approved these changes

Assignees

@MackinnonBuck MackinnonBuck

Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Milestone
8.0-preview1
Development

Successfully merging this pull request may close these issues.
6 participants
@JamesNK @BrennanConroy @mkArtakMSFT @javiercn @MackinnonBuck @dougbu