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

perf(plugin-angular): do not run change detections when notifying the event #1422

Open
wants to merge 1 commit into
base: next
Choose a base branch
from
Open

perf(plugin-angular): do not run change detections when notifying the event #1422

wants to merge 1 commit into from

Conversation

arturovt
Copy link

@arturovt arturovt commented May 29, 2021

Angular runs 10 change detection cycles if the notify() method is called within the Angular zone. This drastically affects the performance since we actually shouldn't run those change detections when notifying error events.

This PR adds an ability to avoid running unwanted change detections when notifying the event.

@arturovt arturovt changed the title perf(angular): do not run change detections when notifying the event perf(plugin-angular): do not run change detections when notifying the event May 29, 2021
@xljones
Copy link

xljones commented Jun 2, 2021

Hey @arturovt thanks for the PR, we'll review as priorities allow :)

@xljones xljones added the needs discussion Requires internal analysis/discussion label Jun 2, 2021
@arturovt
Copy link
Author

@xander-jones friendly ping :)

@mattdyoung
Copy link
Contributor

Hi @arturovt

This is still on our radar but when we reviewed your PR the conclusion was that we wouldn't be able to accept it for the time being. We're considering an alternative implementation which would cover handled as well as unhandled events. We'd also need to treat this as a breaking change in case we have any customers relying on aspects of the existing behavior so we're looking to address this as part of our next major release. Thanks again for the contribution and we'll post on this thread with any updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion Requires internal analysis/discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants