fix(core): replace assertion with more intentional error#52234
fix(core): replace assertion with more intentional error#52234alxhub wants to merge 1 commit intoangular:mainfrom
Conversation
There was a problem hiding this comment.
Would we want to create a Error code for this warning ? (and use formatRuntimeError)
This would allow us to have a dedicated error page for this.
There was a problem hiding this comment.
That was our initial intention but we've decided against it. There are cases where we can't be sure if this is an actual error (ex. signals propagating dirty state without actually updating any values rendered in a template).
Having a warn for now sounds like a good balance between correctness and providing developers with useful information.
There was a problem hiding this comment.
Yes, I agree with you.
My point is that we already have warnings with codes, they log a warning but do not throw. (for example https://next.angular.io/errors/NG0506 or https://next.angular.io/errors/NG0912). This could be one too.
There was a problem hiding this comment.
Oh, this is a good point, we should definitively do that!
There was a problem hiding this comment.
I'm not so worried about it as we don't expect this console.warn to live long (it might not even make it until v17 proper).
packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts
Outdated
Show resolved
Hide resolved
packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts
Outdated
Show resolved
Hide resolved
pkozlowski-opensource
left a comment
There was a problem hiding this comment.
LGTM but it will need a bit of cleanup (golden symbols update)
904df87 to
9e1c7e5
Compare
Issue angular#50320 shows that in some cases, updating a signal that's a dependency of a template during change detection of that template can have several adverse effects. This can happen, for example, if the signal is set during the lifecycle hook of a directive within the same template that reads the signal. This can cause a few things to happen: * Straightforwardly, it can cause `ExpressionChanged` errors. * Surprisingly, it can cause an assertion within the `ReactiveLViewConsumer` to fail. * Very surprisingly, it can cause change detection for an `OnPush` component to stop working. The root cause of these later behaviors is subtle, and is ultimately a desync between the reactive graph and the view tree's notion of "dirty" for a given view. This will be fixed with further work planned for change detection to handle such updates directly. Until then, this commit improves the DX through two changes: 1. The mechanism of "committing" `ReactiveLViewConsumer`s to a view is changed to use the `consumerOnSignalRead` hook from the reactive graph. This prevents the situation which required the assertion in the first place. 2. A `console.warn` warning is added when a view is marked dirty via a signal while it's still executing. The warning informs users that they're pushing data against the direction of change detection, risking `ExpressionChanged` or other issues. It's a warning and not an error because the check is overly broad and captures situations where the application would not actually break as a result, such as if a `computed` marked the template dirty but still returned the same value.
9e1c7e5 to
eb6d840
Compare
|
@jessicajaniuk #52414 has the same size-tracking "issue". |
|
This PR was merged into the repository by commit bdd61c7. |
|
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. |
Issue angular#50320 shows that in some cases, updating a signal that's a dependency of a template during change detection of that template can have several adverse effects. This can happen, for example, if the signal is set during the lifecycle hook of a directive within the same template that reads the signal. This can cause a few things to happen: * Straightforwardly, it can cause `ExpressionChanged` errors. * Surprisingly, it can cause an assertion within the `ReactiveLViewConsumer` to fail. * Very surprisingly, it can cause change detection for an `OnPush` component to stop working. The root cause of these later behaviors is subtle, and is ultimately a desync between the reactive graph and the view tree's notion of "dirty" for a given view. This will be fixed with further work planned for change detection to handle such updates directly. Until then, this commit improves the DX through two changes: 1. The mechanism of "committing" `ReactiveLViewConsumer`s to a view is changed to use the `consumerOnSignalRead` hook from the reactive graph. This prevents the situation which required the assertion in the first place. 2. A `console.warn` warning is added when a view is marked dirty via a signal while it's still executing. The warning informs users that they're pushing data against the direction of change detection, risking `ExpressionChanged` or other issues. It's a warning and not an error because the check is overly broad and captures situations where the application would not actually break as a result, such as if a `computed` marked the template dirty but still returned the same value. PR Close angular#52234
Issue #50320 shows that in some cases, updating a signal that's a dependency of a template during change detection of that template can have several adverse effects. This can happen, for example, if the signal is set during the lifecycle hook of a directive within the same template that reads the signal.
This can cause a few things to happen:
ExpressionChangederrors.ReactiveLViewConsumerto fail.OnPushcomponent to stop working.The root cause of these later behaviors is subtle, and is ultimately a desync between the reactive graph and the view tree's notion of "dirty" for a given view. This will be fixed with further work planned for change detection to handle such updates directly. Until then, this commit improves the DX through two changes:
The mechanism of "committing"
ReactiveLViewConsumers to a view is changed to use theconsumerOnSignalReadhook from the reactive graph. This prevents the situation which required the assertion in the first place.A
console.warnwarning is added when a view is marked dirty via a signal while it's still executing.The warning informs users that they're pushing data against the direction of change detection, risking
ExpressionChangedor other issues. It's a warning and not an error because the check is overly broad and captures situations where the application would not actually break as a result, such as if acomputedmarked the template dirty but still returned the same value.