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(ivy): do not invoke change detection for destroyed views #34241

Closed

Conversation

@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Dec 5, 2019

Prior to this commit, calling change detection for destroyed views resulted in errors being thrown in some cases. This commit adds a check to make sure change detection is skipped in case a given view is marked as destroyed.

Note: I also ran micro benchmarks and the main observation is that noop_change_detection one is 2-3% slower, which is a bit surprising since the operation that this PR adds should be cheap. Other benchmarks have no perf changes.

This PR resolves FW-1636.

PR Type

What kind of change does this PR introduce?

  • Bugfix

Does this PR introduce a breaking change?

  • Yes
  • No
Prior to this commit, calling change detection for destroyed views resulted in errors being thrown in some cases. This commit adds a check to make sure change detection is invoked for non-destroyed views only.
@ngbot ngbot bot added this to the needsTriage milestone Dec 5, 2019
@googlebot googlebot added the cla: yes label Dec 5, 2019
@AndrewKushnir AndrewKushnir marked this pull request as ready for review Dec 6, 2019
@AndrewKushnir AndrewKushnir requested a review from angular/fw-core as a code owner Dec 6, 2019
@AndrewKushnir

This comment has been minimized.

Copy link
Contributor Author

AndrewKushnir commented Dec 6, 2019

Copy link
Member

pkozlowski-opensource left a comment

LGTM. I'm a bit sad that we are adding more checks to refreshView but we shouldn't block a bug fix on this. I've created a follow up issue for the future refactoring work: #34285

AndrewKushnir added a commit that referenced this pull request Dec 6, 2019
Prior to this commit, calling change detection for destroyed views resulted in errors being thrown in some cases. This commit adds a check to make sure change detection is invoked for non-destroyed views only.

PR Close #34241
josephperrott added a commit to josephperrott/angular that referenced this pull request Dec 11, 2019
…#34241)

Prior to this commit, calling change detection for destroyed views resulted in errors being thrown in some cases. This commit adds a check to make sure change detection is invoked for non-destroyed views only.

PR Close angular#34241
@angular-automatic-lock-bot

This comment has been minimized.

Copy link

angular-automatic-lock-bot bot commented Jan 6, 2020

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 Jan 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants
You can’t perform that action at this time.