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
refactor(core): decouple effects from change detection #51049
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.
Excellent start @alxhub !
I've left some comments as discussion points, let's catch up on those.
oh oh, not merged into 17.next.1 😿 |
5679430
to
f1a9ece
Compare
f1a9ece
to
29244a6
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.
LGTM!
29244a6
to
5d5f2c7
Compare
5d5f2c7
to
743f454
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.
LGTM
Reviewed-for: size-tracking
Reviewed-for: fw-core
743f454
to
d5b4271
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.
reviewed-for: public-api
Previously effects were queued as they became dirty, and this queue was flushed at various checkpoints during the change detection cycle. The result was that change detection _was_ the effect runner, and without executing CD, effects would not execute. This leads a particular tradeoff: * effects are subject to unidirectional data flow (bad for dx) * effects don't cause a new round of CD (good/bad depending on use case) * effects can be used to implement control flow efficiently (desirable) This commit changes the scheduling mechanism. Effects are now scheduled via the microtask queue. This changes the tradeoffs: * effects are no longer limited by unidirectional data flow (easy dx) * effects registered in the Angular zone will trigger CD after they run (same as `Promise.resolve` really) * the public `effect()` type of effect probably isn't a good building block for our built-in control flow, and we'll need a new internal abstraction. As `effect()` is in developer preview, changing the execution timing is not considered breaking even though it may impact current users.
d5b4271
to
6119bbc
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.
Reviewed-for: public-api
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
Reviewed-for: public-api
Reviewed-for: fw-core
This PR was merged into the repository by commit 38c9f08. |
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. |
Previously effects were queued as they became dirty, and this queue was flushed at various checkpoints during the change detection cycle. The result was that change detection _was_ the effect runner, and without executing CD, effects would not execute. This leads a particular tradeoff: * effects are subject to unidirectional data flow (bad for dx) * effects don't cause a new round of CD (good/bad depending on use case) * effects can be used to implement control flow efficiently (desirable) This commit changes the scheduling mechanism. Effects are now scheduled via the microtask queue. This changes the tradeoffs: * effects are no longer limited by unidirectional data flow (easy dx) * effects registered in the Angular zone will trigger CD after they run (same as `Promise.resolve` really) * the public `effect()` type of effect probably isn't a good building block for our built-in control flow, and we'll need a new internal abstraction. As `effect()` is in developer preview, changing the execution timing is not considered breaking even though it may impact current users. PR Close angular#51049
Previously effects were queued as they became dirty, and this queue was flushed at various checkpoints during the change detection cycle. The result was that change detection was the effect runner, and without executing CD, effects would not execute. This leads a particular tradeoff:
This commit changes the scheduling mechanism. Effects are now scheduled via the microtask queue. This changes the tradeoffs:
Promise.resolve
really)effect()
type of effect probably isn't a good building block for our built-in control flow, and we'll need a new internal abstraction.As
effect()
is in developer preview, changing the execution timing is not considered breaking even though it may impact current users.