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.

Sign up for GitHub

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

Angular: Run setProps in the NgZone #12382

Merged
merged 6 commits into from
Dec 1, 2020
Merged

Angular: Run setProps in the NgZone #12382

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0c58bd4
Run setProps in the NgZone
Marklb Sep 4, 2020
c1f7f0a
Add semicolon
Marklb Sep 4, 2020
6f8e63e
Merge branch 'next' into marklb/setprops-in-ngzone
Marklb Nov 18, 2020
915f16f
Re-emit story data in ngZone
Marklb Nov 18, 2020
b9ba7a8
Fix lint error
Marklb Nov 21, 2020
b776663
Merge branch 'next' into marklb/setprops-in-ngzone
gaetanmaisse Nov 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion app/angular/src/client/preview/angular/components/app.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
SimpleChanges,
SimpleChange,
ChangeDetectorRef,
NgZone,
} from '@angular/core';
import { Observable, Subscription } from 'rxjs';
import { first } from 'rxjs/operators';
Expand All @@ -36,6 +37,7 @@ export class AppComponent implements OnInit, OnDestroy {
constructor(
private cfr: ComponentFactoryResolver,
private changeDetectorRef: ChangeDetectorRef,
private ngZone: NgZone,
@Inject(STORY) private data: Observable<StoryFnAngularReturnType>
) {}

Expand All @@ -51,7 +53,7 @@ export class AppComponent implements OnInit, OnDestroy {
);

this.subscription = this.data.subscribe((newData) => {
this.setProps(instance, newData);
this.ngZone.run(() => this.setProps(instance, newData));
Copy link
Member

Choose a reason for hiding this comment

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

Is it really necessary to call ngZone.run? I thought the two following lines already take care that the updated props are properly updated by the change detection

        childChangeDetectorRef.markForCheck();
        // Must detect changes on the current component in order to update any changes in child component's @HostBinding properties (angular/angular#22560)
        this.changeDetectorRef.detectChanges();

Though, I must admit I rarely need to leave or re-enter ngZone.

Do you have an example of what does not work right now and would work after this change?

Copy link
Member Author

@Marklb Marklb Oct 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

That is correct that updates made to the props during that tick would get detected, but this is about functions initialized during the tick.

I am not an expert on how the zones work, so I may not be explaining it exactly right. For change detection to know when to trigger a detection tick, it hooks functions like addEventListener, setTimeout, Promises and I'm sure some others. They only get hooked while inside a zone, so when a knob updates a prop the channel triggers the event from a non-hooked function and we update the props without being in the zone. Anything changed from setting the props would be detected, since change detection was manually triggered, but if we subscribe to an output then that event will not be hooked, so when the output EventEmitter emits, Angular doesn't know about it and will not trigger a change detection tick.

I do think I may be doing to much in the zone by calling it there, instead of maybe the parts inside setProps that interact with the component, but I will try to come up with an example that is more clear. The one that caused me to recognize the problem is in this comment. The click event is hooked, since the initial setup is done in the zone, so clicking the button is detected. If the visible knob is clicked, the button is removed and if the knob is clicked again it will correctly show the button again, since change detection was manually triggered after setting the props. If you click the button in the story, the click event no longer trigger's a change detection tick, since the subscription was done outside the zone.

childChangeDetectorRef.markForCheck();
// Must detect changes on the current component in order to update any changes in child component's @HostBinding properties (angular/angular#22560)
this.changeDetectorRef.detectChanges();
Expand Down