Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Components auth: basic services and components #10227
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
Components auth: basic services and components #10227
Changes from all commits
eeda567
031ba22
aa07dbc
296e1a4
0143c3b
1fd4c08
8a21d3b
cfdab41
cfa81b9
0bd49e6
553f9f4
d9e5c4e
b05a7d3
3679217
99ee6f7
3d20454
5f95d3d
f78d427
12bd326
d93774b
dcc7e11
4eef170
4c78ded
3f1143f
12a2f36
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@SteveSandersonMS - something else I thought of late (sorry).
These need to go into the manually maintained section of the ref assembly. The reason why is that ref assemblies don't preserve the setter if it's non-public. You can use cc1b294 as an example.
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.
Great reminder - thanks. This should be covered in today's PR in this commit: ad9ac55
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.
nit: fields -> constructors -> properties
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.
OK. Rather than go through another multi-hour CI process to make this tweak, I'm going to include it in my next auth PR.
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.
technically not correct, it returns "a task that blah blah blah"
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.
OK. Rather than go through another multi-hour CI process to make this tweak, I'm going to include it in my next auth PR.
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.
Is this actually needed now that
NotifyAuthenticationStateChanged
is there?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.
Well spotted. Rather than go through another multi-hour CI process to make this tweak, I'm going to include it in my next auth PR.
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.
Food for thought. It appears that we want to (and support) raising this event on any thread. Do we want to document that as supported? Or do we want to try and make it enforced that you raise this on the sync context.
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.
I don’t think we want that. The callback should be responsible for calling Invoke or InvokeAsync when handling the event.
I’m wondering how good our support story is when a component calls Invoke
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.
It's an interesting question. We could make the
AuthenticationStateProvider
aware of the sync context (it's already a scoped service), and we could use an async-event pattern (Task
-returning delegates) so that all the handlers would be async functions running on the sync context, whether or not they need to be.However, the
AuthenticationStateProvider
is intended as a low-level service that developers will commonly only be the producer of (if implementing a custom authentication system), not the consumer. To consume the authentication state, developers will typically use either the cascaded value or the<AuthorizeView>
, both of which already marshal the flow onto the sync context for you.So, since in common usage patterns this won't be an issue anyway, I'd prefer to keep the
AuthenticationStateProvider
simple and independent of this, letting any developers who choose to use this low-level service deal with their ownInvoke
/InvokeAsync
as Javier mentions, like they would when subscribing to any other service that's separate from the rendering system.