Skip to content
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

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

Closed
wants to merge 1 commit into from

Conversation

alxhub
Copy link
Member

@alxhub alxhub commented Oct 27, 2023

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:

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

  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.

@alxhub alxhub added PullApprove: disable action: merge The PR is ready for merge by the caretaker target: rc This PR is targeted for the next release-candidate labels Oct 27, 2023
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.
@alxhub
Copy link
Member Author

alxhub commented Oct 27, 2023

This PR was merged into the repository by commit 384d7aa.

@alxhub alxhub closed this Oct 27, 2023
alxhub added a commit that referenced this pull request Oct 27, 2023
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:

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 #52427
@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 Nov 27, 2023
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 PullApprove: disable target: rc This PR is targeted for the next release-candidate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant