Skip to content

Conversation

@crisbeto
Copy link
Member

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.

@crisbeto crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer area: core Issues related to the framework runtime target: patch This PR is targeted for the next patch release labels Jul 15, 2024
@ngbot ngbot bot modified the milestone: Backlog Jul 15, 2024
Copy link
Member Author

@crisbeto crisbeto Jul 15, 2024

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.

Copy link
Member

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.

Copy link
Member Author

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?

@crisbeto crisbeto requested a review from mmalerba July 15, 2024 03:48
@crisbeto crisbeto marked this pull request as ready for review July 15, 2024 03:48
@crisbeto crisbeto requested a review from thePunderWoman July 15, 2024 03:50
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.
@crisbeto crisbeto force-pushed the 56979/destroy-hooks branch from 7e07c44 to 175f040 Compare July 15, 2024 06:44
Copy link
Contributor

@thePunderWoman thePunderWoman left a comment

Choose a reason for hiding this comment

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

LGTM

@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jul 16, 2024
@crisbeto crisbeto removed the request for review from mmalerba July 16, 2024 15:19
@atscott
Copy link
Contributor

atscott commented Jul 16, 2024

This PR was merged into the repository by commit e504ad9.

The changes were merged into the following branches: main, 18.1.x

@atscott atscott closed this in e504ad9 Jul 16, 2024
atscott pushed a commit that referenced this pull request Jul 16, 2024
…#56981)

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.

PR Close #56981
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Aug 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[18.1] AfterNextRender only invokes first phase callback when provided multiple via new API

4 participants