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

Angular: Run setProps in the NgZone #12382

Merged
merged 6 commits into from Dec 1, 2020

Conversation

Marklb
Copy link
Member

@Marklb Marklb commented Sep 4, 2020

Issue: #7242

Since Storybook doesn't run stories in NgZone, this sets the props in the zone to allow Angular change detection to detect it.

Ideally the manual change detection calls should avoid the need to enter the zone. What I think may be happening is that when something outside the zone, such as a knob, causes the props to change outside the zone, it is causing things within the component to get initialized without getting hooked by NgZone. So, when something like a button gets created while outside the zone, like when a knob toggles it with *ngIf, the click events probably don't cause a change when clicking the button, because the click event wouldn't have gotten hooked by the zone.

What I did

Emit story data in the NgZone.

How to test

  • Is this testable with Jest or Chromatic screenshots? Not sure yet
  • Does this need a new example in the kitchen sink apps? No
  • Does this need an update to the documentation? No

@shilman shilman added this to the 6.1 maintenance milestone Sep 4, 2020
@shilman
Copy link
Member

shilman commented Sep 4, 2020

@kroeder can you review?

@@ -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

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.

@kroeder
Copy link
Member

kroeder commented Oct 4, 2020

Since Storybook doesn't run stories in NgZone, this sets the props in the zone to allow Angular change detection to detect it.

Storybook does not but the components storybook uses do. I might be wrong! Can you go a little bit more into detail about why you think components are running outside of NgZone?

@yannbf yannbf removed their assignment Oct 10, 2020
@kenjiqq
Copy link

kenjiqq commented Nov 12, 2020

We have this same problem using addon-controls. We have a case similar to the one mentioned in #7242 where one of the controls are toggling a *ngIf of a component that has a button with a click callback. After using the control to hide and show the button it will no longer work to click on the button properly, because the click callback is running outside of angular zone. We tried using the fix in the PR and that solved the problem.

I can explain why this is running outside of a zone.
In app.component.ts @Inject(STORY) private data: Observable<StoryFnAngularReturnType> is subscribed to and when that emits properties are updated.
This subject is defined in app/angular/src/client/preview/angular/helpers.ts and the renderNgApp function is what is calling next on the subject.
However this function is called from from react code in app/angular/src/client/preview/index.ts (using the render.ts file) and that is outside of Angulars Zone.
This leads to the fact that any code that gets executed based on the property changes are running outside the angular zone and therefor any new listeners that the code sets up will forever be outside the angular zone as well.

I think the most correct solution would probabaly be to wrap the storyData.next(storyFn()); in renderNgApp in a zone.run() function so that any other code that also subscribes to the subject will always run in the angular zone. But just for the props problem this PR works as well.

@Mortefal
Copy link

@kroeder we need this for our system, and it seems like @kenborge 's solution would solve it. Is this something that will be merged? :)

@Marklb
Copy link
Member Author

Marklb commented Nov 15, 2020

@kenborge Are you suggesting that the subject emit in the zone for user's code injecting STORY?

If it was decided to do that, we would probably need to re-emit inside the zone, since I don't think the injector is guaranteed to be available to get the NgZone reference in renderNgApp, which can be done though. I am deciding if that should be done. Does it make more sense to have that emit in the zone or expect it to be outside the zone, like native events or non-Angular observables that haven't been wrapped to re-emit in the zone? The reason I think it makes sense to not be in the zone is that the storyFn changing does not necessarily mean that something the change detector needs to know about happened, but maybe the way we are using it that would be assumed for anything using the injected STORY, instead of Storybook's api.

@Mortefal Do you have an example of a situation that would need @kenborge's suggestion or just agreeing that it would be a better solution?

@kenjiqq
Copy link

kenjiqq commented Nov 16, 2020

@Marklb The reason i suggested wrapping the emit inside the zone is that you are providing the subject in then angular module as Storytoken, so anything that then uses this provided Story token shouldn't have to know this will be called from outside of angular zone. That's just general Angular best practices.
But if that is a problem in this case it could also be handled any place you subscribe to it by using a zone.run there ( that is what this PR is currently doing).

I made a small repo to show the actual problem. https://github.com/kenborge/storybook-zone-problem

  • If you start storybook here there is a button wrapper story.
  • Clicking the button will output an action with the text: running in zone true
  • If you then go to controls and and toggle showButton to false the button is removed with a ngIf, then turn it back to true.
  • Now click the button again and it will output running in zone false.

That means the button click callback in the button component is now no longer running in the angular zone

@Marklb
Copy link
Member Author

Marklb commented Nov 18, 2020

@kenborge After thinking about it some more, I agree with you that it should probably emit that observable in the zone and leave it up to the subscriber to exit the zone if necessary. That should avoid inconsistency between first and subsequent emissions.

Does this PR look better? Instead of providing the subject directly, I provided a factory that depends on NgZone and creates a new observable to emit in the zone. I tried to basically create an operator that emits the value to the next subscriber inside the zone.

@shilman
Copy link
Member

shilman commented Nov 21, 2020

@Marklb @kroeder is this ready to go?

@shilman shilman changed the title Run setProps in the NgZone Angular: Run setProps in the NgZone Nov 21, 2020
@Marklb
Copy link
Member Author

Marklb commented Nov 21, 2020

I would like someone else to review the most recent change to this, where I added the storyDataFactory, but I have been trying to find a problem and don't see one yet.

The only potential issue I see is maybe to many change detection ticks being triggered on initial render, but I am seeing the same number of checks with or without this change. So, even if that was a problem, I don't think this would be causing it.

@Marklb Marklb mentioned this pull request Nov 24, 2020
2 tasks
@ThibaudAV
Copy link
Contributor

ThibaudAV commented Nov 24, 2020

If my test is correct with this ex.
I think that just add this in the angular-polyfills.ts fix the bug.
import 'zone.js/dist/zone-patch-rxjs';

if you can also check from your side that I am not wrong.
This would avoid a dependency on zone.js in the code 🤷‍♂️

@Marklb
Copy link
Member Author

Marklb commented Nov 25, 2020

@ThibaudAV I looked at the polyfil approach and it does seem to work, but I am trying to understand the difference. It may be some config or dependency difference, but I am getting different results. If I provide the factory, which this PR currently does, then I seem to be getting the same result in my library project and Storybook's example project. If I use the patch, then I get a different result in my project than Storybook's example project.

I was looking at this, which basically says the same thing as the documentation, but in the comments is a stackblitz example: https://stackblitz.com/edit/angular-zone-rxjs-patch-test

I created the following story from that stackblitz. In examples/angular-cli I am not seeing a difference between using the this PR's current solution and the patch, but in my library project I get the same result as that stackblitz, which is different.

@Component({
  selector: 'story-example',
  template: `
    <button type="button" (click)="createObsOutsideCallNextInside()">
      Create obsoutside and call net inside
    </button>
  `
})
class StoryExample {

  constructor(private _ngZone: NgZone) { }

  where() {
    return NgZone.isInAngularZone() ? 'inside' : 'outside'
  }

  createObsOutsideCallNextInside() {
    let observable, sub

    this._ngZone.runOutsideAngular(() => {
      console.log('Creating obs', this.where())
      observable = new Observable(subscriber => sub = subscriber)
      observable = observable.pipe(map(val => {
        console.log('First map', this.where())
        return val
      }))
    })

    observable = observable.pipe(map(val => {
      console.log('Second map', this.where())
      return val
    }))

    this._ngZone.runOutsideAngular(() => {
      observable.subscribe(() => {
          console.log('Getting value', this.where())
      })
    })

    console.log('Nexting value', this.where())
    sub.next(1)
  }

}

export const Example = (args) => ({
  props: args,
  component: StoryExample
})

If I use this PR's change in my library project, then I get the result I expected. If I then add the patch also, I get a different result with the story above. Based on that, the patch seems to be a different opt-in functionality.

@Marklb
Copy link
Member Author

Marklb commented Nov 27, 2020

The current change is the safest option, in my opinion.

The way 'zone.js/dist/zone-patch-rxjs' works does make sense, and is how I used to think NgZone worked, but since that isn't the default, I don't know about forcing it to work that way. I had already got used to the source determining if it was in a Zone, not the Zone the callback was created in, before I knew the patch existed.

If the user wants the zones to work like 'zone.js/dist/zone-patch-rxjs' they should still be able to use it by importing it in preview.ts, since it is a global patch. It seems to get applied when I add it to my preview.ts. This PR doesn't seem to affect the patch either, because I used this PR with the patch and got the patch's functionality.

Copy link
Contributor

@ThibaudAV ThibaudAV left a comment

Choose a reason for hiding this comment

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

Ok @Marklb agree with you.
🚀 for me

Copy link
Member

@gaetanmaisse gaetanmaisse left a comment

Choose a reason for hiding this comment

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

LGTM

@gaetanmaisse
Copy link
Member

I updated the branch with next and pushed to trigger the CI as the last run hasn't completed correctly.

@shilman shilman merged commit 0e7f50f into storybookjs:next Dec 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants