Skip to content

Conversation

@alxhub
Copy link
Member

@alxhub alxhub commented Aug 19, 2024

No description provided.

@angular-robot angular-robot bot added the area: core Issues related to the framework runtime label Aug 19, 2024
@ngbot ngbot bot added this to the Backlog milestone Aug 19, 2024
@alxhub alxhub force-pushed the appref-dirty-after-render-refactor branch 3 times, most recently from 29f9716 to 25ce6ef Compare August 19, 2024 17:22
@alxhub alxhub added the target: patch This PR is targeted for the next patch release label Aug 19, 2024
@alxhub alxhub requested a review from thePunderWoman August 19, 2024 18:03
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.

One non-blocking comment, but otherwise LGTM!

Copy link
Contributor

Choose a reason for hiding this comment

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

Excuse me, After Render, but I'd like to speak to the Manager.

Copy link
Contributor

Choose a reason for hiding this comment

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

This feels hacky to me. Do we have to do this or can we simply return null and handle it on the call site?

Copy link
Member Author

Choose a reason for hiding this comment

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

We're not returning [undef, undef, undef, undef]. This case handles the single-callback form: afterRender(() => {...}, {phase: X}). The next line stores the callback into the right phase slot in that sequence.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* `AfterRenderPhase.EarlyRead` phase if reading can wait until after the write phase.
* `AfterRenderPhase.Read` phase if reading can wait until after the write phase.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry if I'm missing something - but if you only did this to defer deletion: It is safe to delete items from a set during iteration. It will continue to iterate in the order of insertion.

Copy link
Member Author

@alxhub alxhub Aug 20, 2024

Choose a reason for hiding this comment

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

Good point - cleaned that up a bit, ty! I still have nightmares from my Java days of ConcurrentModificationExceptions

@alxhub alxhub force-pushed the appref-dirty-after-render-refactor branch from 25ce6ef to bb579b0 Compare August 20, 2024 17:59
@alxhub alxhub force-pushed the appref-dirty-after-render-refactor branch from bb579b0 to 075df09 Compare August 21, 2024 20:23
@alxhub
Copy link
Member Author

alxhub commented Aug 21, 2024

Pending TGP :)

@alxhub alxhub force-pushed the appref-dirty-after-render-refactor branch 2 times, most recently from f40d806 to 075df09 Compare August 23, 2024 05:01
alxhub added 2 commits August 22, 2024 22:02
Previously the zoneless scheduler had a concept of whether views needed to
be refreshed or not, based on the notification type that was received. It
tracked this information as a boolean.

This commit refactors things to track dirtiness in `ApplicationRef` itself,
as a `dirtyFlags` field with bits corresponding to either view tree
dirtiness or after-render hooks.
…hases

The `afterRender` infrastructure was first implemented around the idea of
independent, singular hooks. It was later updated to support a spec of
multiple hooks that pass values from one to another as they execute, but the
implementation still worked in terms of singular hooks under the hood. This
creates a number of maintenance issues, and a few bugs. For example, when
one hook fails, further hooks in the pipeline should no longer execute, but
this was hard to ensure under the old design.

This refactoring restructures `afterRender` infrastructure significantly to
introduce the concept of a "sequence", a collection of hooks of different
phases that execute together. Overall, the implementation is simplified
while making it more resilient to issues and future use cases, such as the
upcoming `afterRenderEffect`.

As part of this refactoring, the `internalAfterNextRender` concept is
removed, as well as the unused `queueStateUpdate` concept which used it.
@alxhub alxhub force-pushed the appref-dirty-after-render-refactor branch from 075df09 to 6590492 Compare August 23, 2024 05:02
@alxhub alxhub added action: merge The PR is ready for merge by the caretaker target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels Aug 23, 2024
@alxhub
Copy link
Member Author

alxhub commented Aug 23, 2024

This PR was merged into the repository by commit b80af11.

The changes were merged into the following branches: main

@alxhub alxhub closed this in dabfb6d Aug 23, 2024
alxhub added a commit that referenced this pull request Aug 23, 2024
…hases (#57453)

The `afterRender` infrastructure was first implemented around the idea of
independent, singular hooks. It was later updated to support a spec of
multiple hooks that pass values from one to another as they execute, but the
implementation still worked in terms of singular hooks under the hood. This
creates a number of maintenance issues, and a few bugs. For example, when
one hook fails, further hooks in the pipeline should no longer execute, but
this was hard to ensure under the old design.

This refactoring restructures `afterRender` infrastructure significantly to
introduce the concept of a "sequence", a collection of hooks of different
phases that execute together. Overall, the implementation is simplified
while making it more resilient to issues and future use cases, such as the
upcoming `afterRenderEffect`.

As part of this refactoring, the `internalAfterNextRender` concept is
removed, as well as the unused `queueStateUpdate` concept which used it.

PR Close #57453
alxhub added a commit to alxhub/angular that referenced this pull request Aug 23, 2024
Previously the zoneless scheduler had a concept of whether views needed to
be refreshed or not, based on the notification type that was received. It
tracked this information as a boolean.

This commit refactors things to track dirtiness in `ApplicationRef` itself,
as a `dirtyFlags` field with bits corresponding to either view tree
dirtiness or after-render hooks.

PR Close angular#57453
alxhub added a commit to alxhub/angular that referenced this pull request Aug 23, 2024
…hases (angular#57453)

The `afterRender` infrastructure was first implemented around the idea of
independent, singular hooks. It was later updated to support a spec of
multiple hooks that pass values from one to another as they execute, but the
implementation still worked in terms of singular hooks under the hood. This
creates a number of maintenance issues, and a few bugs. For example, when
one hook fails, further hooks in the pipeline should no longer execute, but
this was hard to ensure under the old design.

This refactoring restructures `afterRender` infrastructure significantly to
introduce the concept of a "sequence", a collection of hooks of different
phases that execute together. Overall, the implementation is simplified
while making it more resilient to issues and future use cases, such as the
upcoming `afterRenderEffect`.

As part of this refactoring, the `internalAfterNextRender` concept is
removed, as well as the unused `queueStateUpdate` concept which used it.

PR Close angular#57453
alxhub added a commit that referenced this pull request Aug 23, 2024
…7504)

Previously the zoneless scheduler had a concept of whether views needed to
be refreshed or not, based on the notification type that was received. It
tracked this information as a boolean.

This commit refactors things to track dirtiness in `ApplicationRef` itself,
as a `dirtyFlags` field with bits corresponding to either view tree
dirtiness or after-render hooks.

PR Close #57453

PR Close #57504
alxhub added a commit that referenced this pull request Aug 23, 2024
…hases (#57453) (#57504)

The `afterRender` infrastructure was first implemented around the idea of
independent, singular hooks. It was later updated to support a spec of
multiple hooks that pass values from one to another as they execute, but the
implementation still worked in terms of singular hooks under the hood. This
creates a number of maintenance issues, and a few bugs. For example, when
one hook fails, further hooks in the pipeline should no longer execute, but
this was hard to ensure under the old design.

This refactoring restructures `afterRender` infrastructure significantly to
introduce the concept of a "sequence", a collection of hooks of different
phases that execute together. Overall, the implementation is simplified
while making it more resilient to issues and future use cases, such as the
upcoming `afterRenderEffect`.

As part of this refactoring, the `internalAfterNextRender` concept is
removed, as well as the unused `queueStateUpdate` concept which used it.

PR Close #57453

PR Close #57504
@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 Sep 23, 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: minor This PR is targeted for the next minor release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants