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(component): reset state / trigger CD only if necessary #3328

Merged

Conversation

markostanimirovic
Copy link
Member

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[x] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[x] Other... Please describe: Performance improvements

What is the current behavior?

  1. When the first observable that emits the value synchronously arrives, the change detection will be triggered two times. This causes an error when changeDetectorRef.detectChanges is called in zoneless environment (reported in Push pipe calls detectChanges before setting value #3327).
  2. When another observable that emits the value synchronously arrives, the same will happen.

Closes #3327

What is the new behavior?

For both scenarios 👆 the change detection will be triggered once. The behavior for observables that emit asynchronously will remain the same..

Does this PR introduce a breaking change?

[ ] Yes
[x] No

@netlify
Copy link

netlify bot commented Feb 23, 2022

Deploy Preview for ngrx-io ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 5ae9f31
🔍 Latest deploy log https://app.netlify.com/sites/ngrx-io/deploys/6255e6e3f03faa00085b0504
😎 Deploy Preview https://deploy-preview-3328--ngrx-io.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@markostanimirovic markostanimirovic force-pushed the perf/component/change-detection branch 2 times, most recently from a890669 to c5a9de9 Compare February 23, 2022 22:32
@markostanimirovic markostanimirovic force-pushed the perf/component/change-detection branch 4 times, most recently from 7736688 to 0a26a99 Compare March 1, 2022 22:51
@markostanimirovic markostanimirovic changed the title perf(component): trigger change detection once when new observable emits synchronously perf(component): reset state / trigger CD only if necessary Mar 1, 2022
Copy link
Member

@brandonroberts brandonroberts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM on code, but if this changes the current behavior of two detection cycles to one, might be an unintentional breaking change for current users.

modules/component/src/core/render-creator.ts Outdated Show resolved Hide resolved
@markostanimirovic
Copy link
Member Author

LGTM on code, but if this changes the current behavior of two detection cycles to one, might be an unintentional breaking change for current users.

Sure, let's postpone this change for v14.

I also have in mind a few more improvements, such as better typing for LetDirective, which would also be a breaking change.

Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

@markostanimirovic markostanimirovic force-pushed the perf/component/change-detection branch 2 times, most recently from f118acc to 2c71c61 Compare April 8, 2022 13:21
@markostanimirovic markostanimirovic marked this pull request as ready for review April 8, 2022 13:38
Comment on lines +178 to +179
ngOnInit(): void {
this.subscription.add(
this.renderEventManager.handlePotentialObservableChanges().subscribe()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After playing around with the suspense template, I realized that the potential observable subscription has to be moved to the ngOnInit method in order to get initial input values on directive initialization.

@brandonroberts brandonroberts merged commit f5b055b into ngrx:master May 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Push pipe calls detectChanges before setting value
4 participants