-
Notifications
You must be signed in to change notification settings - Fork 27k
fix(core): not all callbacks running when registered at the same time #56981
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
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.
Note: the assumption here is that we'll always run through all the phases in their defined order once, before we run other phases. E.g. we can't have two read calls in a row before first hitting an earlyRead for a setup like afterRender({read: fn, earlyRead: fn}). I think that's true based on the code that executes them, but it's worth calling out.
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.
Somewhat of a tangent: one thing we discussed just now is how this behaves w.r.t. an exception in one of the phases. Currently I believe this would continue to run the subsequent phases, passing the resulting state of the phase that ran prior to the one that threw an exception (based on how pipelinedArgs is updated after the callback has executed).
I wonder if we should cancel all subsequent callbacks in case of an exception, effectively dropping callbacksLeftToRun to 0 to let it clean up. It probably makes sense for afterRender to have similar behavior so that would need some sort of a different approach.
The above is kind of out of scope because this PR makes things clearly more correct than what we have today, but it would be beneficial to have tests for the behavior in the presence of exceptions.
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.
Yeah agreed that it would make more sense that the chain gets interrupted if one of the callbacks throws, although the current behavior seems quite deliberate to me (reading the code). Maybe @devknoll has context on it?
Fixes that only the first callback was firing when multiple are registered in the same call to `afterNextRender`, e.g. `afterNextRender({earlyRead: fn, read: fn});`
Fixes angular#56979.
7e07c44 to
175f040
Compare
thePunderWoman
left a comment
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.
LGTM
|
This PR was merged into the repository by commit e504ad9. The changes were merged into the following branches: main, 18.1.x |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Fixes that only the first callback was firing when multiple are registered in the same call to
afterNextRender, e.g.afterNextRender({earlyRead: fn, read: fn});Fixes #56979.