fix(core): replace assertion with more intentional error #52234

alxhub · 2023-10-16T21:22:23Z

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:

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:

The mechanism of "committing" ReactiveLViewConsumers 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.
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.

JeanMeche · 2023-10-16T21:53:42Z

packages/core/src/render3/reactive_lview_consumer.ts

-            'Updating a signal during template or host binding execution is not allowed.');
-    markViewDirty(node.lView!);
+    if (ngDevMode && node.isRunning) {
+      console.warn(


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.

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.

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.

Oh, this is a good point, we should definitively do that!

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

pkozlowski-opensource

LGTM but it will need a bit of cleanup (golden symbols update)

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.

jessicajaniuk

That's a sizeable increase to aio main, but I'm guessing it's not from this PR.

reviewed-for: size-tracking

JeanMeche · 2023-10-27T17:39:20Z

@jessicajaniuk #52414 has the same size-tracking "issue".

alxhub · 2023-10-27T18:41:23Z

This PR was merged into the repository by commit bdd61c7.

angular-automatic-lock-bot · 2023-11-27T00:09:03Z

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.}

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

JeanMeche reviewed Oct 16, 2023

View reviewed changes

pkozlowski-opensource added action: review The PR is still awaiting reviews from at least one requested reviewer area: core Issues related to the framework runtime labels Oct 17, 2023

ngbot bot modified the milestone: Backlog Oct 17, 2023

pkozlowski-opensource reviewed Oct 17, 2023

View reviewed changes

packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts Outdated Show resolved Hide resolved

pkozlowski-opensource reviewed Oct 17, 2023

View reviewed changes

packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts Outdated Show resolved Hide resolved

pkozlowski-opensource approved these changes Oct 17, 2023

View reviewed changes

pkozlowski-opensource added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 17, 2023

pkozlowski-opensource modified the milestones: Backlog, v17-final Oct 17, 2023

alxhub force-pushed the signals-cd/error branch from 904df87 to 9e1c7e5 Compare October 26, 2023 23:06

alxhub added action: merge The PR is ready for merge by the caretaker target: rc This PR is targeted for the next release-candidate and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Oct 26, 2023

alxhub force-pushed the signals-cd/error branch from 9e1c7e5 to eb6d840 Compare October 27, 2023 16:20

pullapprove bot requested review from atscott and jessicajaniuk October 27, 2023 16:20

alxhub added target: minor This PR is targeted for the next minor release and removed target: rc This PR is targeted for the next release-candidate labels Oct 27, 2023

jessicajaniuk approved these changes Oct 27, 2023

View reviewed changes

jessicajaniuk removed the request for review from atscott October 27, 2023 17:38

JeanMeche mentioned this pull request Oct 27, 2023

Project root node of @if and @for blocks #52414

Closed

alxhub closed this in bdd61c7 Oct 27, 2023

angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): replace assertion with more intentional error #52234

fix(core): replace assertion with more intentional error #52234

alxhub commented Oct 16, 2023

JeanMeche Oct 16, 2023 •

edited

pkozlowski-opensource Oct 17, 2023

JeanMeche Oct 17, 2023

pkozlowski-opensource Oct 17, 2023

alxhub Oct 26, 2023

pkozlowski-opensource left a comment

jessicajaniuk left a comment •

edited

JeanMeche commented Oct 27, 2023 •

edited

alxhub commented Oct 27, 2023

angular-automatic-lock-bot bot commented Nov 27, 2023

fix(core): replace assertion with more intentional error #52234

fix(core): replace assertion with more intentional error #52234

Conversation

alxhub commented Oct 16, 2023

JeanMeche Oct 16, 2023 • edited

Choose a reason for hiding this comment

pkozlowski-opensource Oct 17, 2023

Choose a reason for hiding this comment

JeanMeche Oct 17, 2023

Choose a reason for hiding this comment

pkozlowski-opensource Oct 17, 2023

Choose a reason for hiding this comment

alxhub Oct 26, 2023

Choose a reason for hiding this comment

pkozlowski-opensource left a comment

Choose a reason for hiding this comment

jessicajaniuk left a comment • edited

Choose a reason for hiding this comment

JeanMeche commented Oct 27, 2023 • edited

alxhub commented Oct 27, 2023

angular-automatic-lock-bot bot commented Nov 27, 2023

JeanMeche Oct 16, 2023 •

edited

jessicajaniuk left a comment •

edited

JeanMeche commented Oct 27, 2023 •

edited