-
Notifications
You must be signed in to change notification settings - Fork 27k
Bundled refactor of ApplicationRef dirtiness & afterRender machinery
#57453
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
29f9716 to
25ce6ef
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.
One non-blocking comment, but otherwise LGTM!
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.
Excuse me, After Render, but I'd like to speak to the Manager.
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.
This feels hacky to me. Do we have to do this or can we simply return null and handle it on the call site?
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.
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.
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.
| * `AfterRenderPhase.EarlyRead` phase if reading can wait until after the write phase. | |
| * `AfterRenderPhase.Read` phase if reading can wait until after the write phase. |
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.
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.
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.
Good point - cleaned that up a bit, ty! I still have nightmares from my Java days of ConcurrentModificationExceptions
25ce6ef to
bb579b0
Compare
bb579b0 to
075df09
Compare
|
Pending TGP :) |
f40d806 to
075df09
Compare
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.
075df09 to
6590492
Compare
|
This PR was merged into the repository by commit b80af11. The changes were merged into the following branches: main |
…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
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
…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
…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
…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
|
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. |
No description provided.