-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Add typed events for all user hooks and legacy events #18225
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.
quick 🚬-test
looking good. Lets do this 🚀
db3bef7
to
5891d4e
Compare
rebased to resolve conflicts |
updated to fix tests |
5891d4e
to
df95437
Compare
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.
Houston, We have a liftoff 🚀
This solves #18331, right? |
Nope. The uses of the old event are still a problem in some cases. I'll try to have a look at that as well. The problem is that we can't migrate those without introducing a breaking change 😢 |
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
df95437
to
6dfb31a
Compare
@ChristophWurst |
Those logs are fine. You only see them with log minimum log levels set. We want them to be visible for app developers so they take action timely. On production instances these lines won't show up. |
@ChristophWurst |
It's not just about the apps. As long as we emit the legacy event for BC reasons this info log will be shown. |
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
Can't make CI happy here. #18348 is a simplified version for 18. Cleanup has to follow later. |
@ChristophWurst Can we close this now? |
Update
see #18348
-> existing code should continue to work, but new code can use the typed events with 18+
Note: I don't get the diff between
Session
andUserManager
hooks as we always emitted on the manager.