-
Notifications
You must be signed in to change notification settings - Fork 596
[Prototype] Rework events (support DI) #1055
Conversation
Hi @HaoK, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! The agreement was validated by .NET Foundation and real humans are currently evaluating your PR. TTYL, DNFBOT; |
Getting rid of the interfaces would be a cool change as we could introduce new events without breaking the contract... but are you sure it's really non-breaking? (I'm tempted to think it's a binary change 😨) |
|
||
if (Options?.Events?.EventsType != null) | ||
{ | ||
var events = services.GetRequiredService(Options.Events.EventsType) as AuthenticationEvents; |
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.
Same remark as in the other PR, doing that at this stage won't allow using scoped event classes. It's something you'd have to do in CreateHandler
or directly in the handler.
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.
Bleh, will require more plumbing then :(
@PinpointTownes this is meant to be a breaking change :) |
@HaoK ah, it would have to wait until 2.0 then, right? |
Yes but we are just validating the design with a prototype right? |
Removed the one time resolution of the events type from the base middleware, and updated JwtBearer and cookie to demonstrate the new pattern: Each events class will have (yet another) new method that hides a |
Sure. But removing the interfaces is orthogonal. The "events type" thingy could be added in a non-breaking way, no?
So, to make things clearer (for me 😄), to use a specific events class, I'd have to do something like that:
... right? It seems odd to me that you have to instantiate a |
Sure but this PR is taking a look at all the events changes we'd like to make, not trying to scope this down since this is more of a prototype for now... In regards to the usage, its a bit clunky due to no lambda so you can't inline:
|
In this case, we could also reconsider aspnet/Options#11 for 2.0. |
Not sure we want to add anything to options regarding this anymore, but we could certainly see if there's anything that makes sense to add to DI to make this pattern easier. |
/// <summary> | ||
/// Base class for all authentication events. | ||
/// </summary> | ||
public class AuthenticationEvents |
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.
Using this as a base class isn't helpful when everybody just overrides it (and awkwardly at that).
Will be replaced as part of #1065 |
Seems fairly seemless, no tests needed to be updated with this change surprisingly... I added one for cookies to pull its events from DI
@Tratcher @PinpointTownes @davidfowl