Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

RXjs and zone.js issue #830

Closed
philjones88 opened this issue Jul 4, 2017 · 33 comments
Closed

RXjs and zone.js issue #830

philjones88 opened this issue Jul 4, 2017 · 33 comments

Comments

@philjones88
Copy link

philjones88 commented Jul 4, 2017

Hello,

I'm developing an Outlook Add-in which essentially is an instance of a Safari browser, this points to an angular 4.2.5 app.

I'm heavily using NGRX Store and RX but I'm hitting issues when I run it inside Outlook rather than developing on Chrome.

This is all using the Angular CLI and JIT not AoT.

Angular version: 4.2.5
Zone.js version: 0.8.12
RX version: 5.4.1
Node version: 8.1

It seems #671 is close to what I'm seeing. But seemingly randomly. I essentially have:

Component:

this.things$ = this.store.select((state) => state.things).distinctUntilChanged();

View:

<div *ngFor="let thing of (things$ | async)">
    <p>Hello {{ thing.name }}!</p>
</div>

Sometimes it won't show the list at all, other times it'll do it.

If I swap back to arrays:

Component:

this.things = [ ];

this.store
     .select((state) => state.things)
     .distinctUntilChanged()
     .map((things) => this.things = things)
     .subscribe();

View:

<div *ngFor="let thing of things">
    <p>Hello {{ thing.name }}!</p>
</div>

This doesn't work at all!

I have to do this in the component:

this.store
     .select((state) => state.things)
     .distinctUntilChanged()
     .map((things) => {
         this.ngZone.run(() => {
             this.things = things;
        });
     })
     .subscribe();

This works every time I reload the page or add-in.

Which to me hints at the RX is somehow escaping the zone on occasion?

@JiaLiPassion
Copy link
Collaborator

@philjones88 , could you post a reproduce repo?

@philjones88
Copy link
Author

@JiaLiPassion You'd have to have Outlook For Mac 2016 or Outlook 2016 (Windows) and be able to install an Add-in. I could provide a CLI based repo with the XML file needed to install the add-in. But it's not something I can plunkr etc as it works fine outside Outlook in Chrome/Safari.

@philjones88
Copy link
Author

A potential cause is if https://github.com/OfficeDev/office-js is causing a conflict with zone.js.

They make you do some silly bootstrapping. You have to wrap your Angular bootstrapping inside their bootstrapping.

I generally disable this for dev on Chrome outside Outlook which might be a reason why it works outside Outlook.

Office.initialize = () => {
   // Your angular bootstrap code here
};

@JiaLiPassion
Copy link
Collaborator

@philjones88 , sure, please provide the CLI side setting files and source, and please tell me how to install it, I resolved some issue for Outlook addin for zone.js before, so I have the basic environment.

@philjones88
Copy link
Author

@JiaLiPassion I'll send you a ZIP and instructions, it's just the UI part but don't really want to post a GH public repo :)

@philjones88
Copy link
Author

@JiaLiPassion isolating this down, it seems to be a safari issue not updating the view with changes, chrome updates fine.

@JiaLiPassion
Copy link
Collaborator

@philjones88 , ok , so maybe safari have some different API just like the issue in #671. I will check it after you send me the repo.

@JiaLiPassion
Copy link
Collaborator

@philjones88 , I didn't find why it not work in safari, but I removed the this.ngZone.runOutsideAngular code, safari will also work.

@philjones88
Copy link
Author

@JiaLiPassion Hmm. That's to make sure the zone gets stable because of Protractor. Interesting find though.

@philjones88
Copy link
Author

@JiaLiPassion google'ing around I found https://stackoverflow.com/questions/42700937/angular2-domevents-rxjs-callbacks-happen-outside-ngzone-on-safari and it talks about adding the polyfill https://github.com/webcomponents/webcomponentsjs ? I'm using the latest CLI and the pollyfills.ts file it generated doesn't mention it.

@JiaLiPassion
Copy link
Collaborator

JiaLiPassion commented Jul 4, 2017

@philjones88 , yeah, I forget about it, you may try that too, sorry for the wrong link, if webcomponentjs involved, you can also check this one, #784.

@philjones88
Copy link
Author

@JiaLiPassion Interesting stuff.

I had a look at the static methods on NgZone and found NgZone.isInAngularZone()

e.g. a striped down version of the code I sent you via email

  public ngOnInit(): void {
    this.logger.debug('ngOnInit');

    this.spaces$ = this.store
      .select((state) => state.activity.spaces)
      .do(() => {
        const inZone = NgZone.isInAngularZone();
        this.logger.debug('spaces$ next => assertInAngularZone', inZone);
      });

    const repeater$ = Observable.of({})
      .do(() => {
        this.logger.debug('Poll action dispatched');
        this.store.dispatch(new activity.PollAction());
      })
      .delay(2000)
      .repeat();

    this.ngZone.runOutsideAngular(() => {
      this.activitySubscription = repeater$.subscribe(
        () => { },
        (error) => {
          this.logger.error('refresh error', error);
        },
        () => this.logger.info('finished')
      );
    });
  }

You'll see console output of:

ngOnInit
Poll action dispatched
UriService => get  8
spaces$ next => assertInAngularZone true
Poll action dispatched
UriService => get  6
spaces$ next => assertInAngularZone false
Poll action dispatched
...

And the UI in safari stick with showing 8.

Strangely even though you get the same output in Chrome, Chrome updates the UI!

I'll work on getting a plunkr or similar of this behaviour.

@philjones88
Copy link
Author

Here's a plnkr to replicate it:

https://embed.plnkr.co/st18pkTPDJtXBpeABNUt/

It'll always be blank as it seems to be executing the observable outside the zone.

As you said, comment on the this.ngZone.runOutsideAngular(() => { lines and it works.

@philjones88 philjones88 changed the title Safari (inside Outlook Add-in) potential zone.js issue RXjs and zone.js issue Jul 5, 2017
@philjones88
Copy link
Author

Seems like https://github.com/angular/zone.js/issues/304 could be related.

@philjones88
Copy link
Author

philjones88 commented Jul 5, 2017

Final comment for a while as I have a workaround for now.

It does seem to be because the observables from the store break out of the zone.

First initial change (before the polling RX kicks in) they all return zone "angular".

When the polling RX kicks in and it's running outside the zone, they are all suddenly inside the "<root>" zone.

My workaround is inside my observables (for the UI async pipe):

this.spaces$ = this.store
  .select((state) => state.spaces)
  .distinctUntilChanged()
  .do((spaces) => {
    this.changeRef.detectChanges(); // Workaround: Make the UI update!
  });

The UI then updates even though they're in the wrong zone. It probably eats CPU cycles but I can't keep being blocked by this :(

@JiaLiPassion
Copy link
Collaborator

@philjones88 , thank you for the detail information and the reproduce plunker , I will continue to try to find out the reason.

@JiaLiPassion
Copy link
Collaborator

@philjones88, I updated plunker, it seems that if you subscribe the observable outside ngZone, then everything belongs to the observable will be outside ngZone , so if you want to run some code outof ngZone, you can write like this. https://embed.plnkr.co/st18pkTPDJtXBpeABNUt/

@philjones88
Copy link
Author

@JiaLiPassion I don't see any changes. I don't think you forked/saves the plnkr?

@JiaLiPassion
Copy link
Collaborator

@philjones88
Copy link
Author

@JiaLiPassion that's not really going to work. The whole idea of the code is to run the polling update logic outside the zone for Protractor.

See:

https://github.com/angular/protractor/blob/master/docs/timeouts.md#angular

And the GH issue:

angular/protractor#3349

@JiaLiPassion
Copy link
Collaborator

@philjones88 , I see, so I think now you have to follow the instructions from here.
https://github.com/angular/protractor/blob/master/docs/timeouts.md#angular

this.ngZone.runOutsideAngular(() => {
  setTimeout(() => {
    // Changes here will not propagate into your view.
    this.ngZone.run(() => {
      // Run inside the ngZone to trigger change detection.
    });
  }, REALLY_LONG_DELAY);
});

runOutsideOfAngular to not wait and run inside of ngZone to update.

@philjones88
Copy link
Author

@JiaLiPassion I've tried something similar with wrapping the observables from the NGRX store in a ngZone.run but that doesn't help. I can't see a combination of my scenario that works. This is still a pretty serious bug for us. I guess we will have to wait till the Angular team add zone scheduling perhaps :(

@mleibman
Copy link
Contributor

mleibman commented Jul 10, 2017

IIUIC, this should get fixed with ReactiveX/rxjs#2266.
For the time being, I'm using this as a workaround:

function patchRxJsObservables() {
  // Patch RxJs observables.
  const originalSubscribe = Observable.prototype.subscribe;

  function maybeWrapFn(fn: any): any {
    return typeof fn === 'function' ? Zone.current.wrap(fn, 'observable') : fn;
  }

  Observable.prototype.subscribe = function(
      observerOrNext?: any, error?: (error: any) => void,
      complete?: () => void): Subscription {
    return originalSubscribe.call(
        this, maybeWrapFn(observerOrNext), maybeWrapFn(error),
        maybeWrapFn(complete));
  };
}

@JiaLiPassion
Copy link
Collaborator

@mleibman , thank you for the information and the walk around, I will test it.

JiaLiPassion added a commit to JiaLiPassion/zone.js that referenced this issue Jul 16, 2017
JiaLiPassion added a commit to JiaLiPassion/zone.js that referenced this issue Jul 19, 2017
@JiaLiPassion
Copy link
Collaborator

@philjones88 , with the new 0.8.15 release, it should work after you load zone-patch-rxjs, I will try to create a sample later.

@JiaLiPassion
Copy link
Collaborator

JiaLiPassion commented Aug 8, 2017

@philjones88 , I have created a test repository based on your plunker, and after zone.js 0.8.17(not released yet), your issue will be resolved. https://github.com/JiaLiPassion/rxjs-zone

@bryanrideshark
Copy link

@JiaLiPassion Any expectation on when zone.js 0.8.17 will be released? I'm not sure, but I think our codebase is also being affected by this issue. Using Phil's workaround for now.

@JiaLiPassion
Copy link
Collaborator

@bryanrideshark , I am not sure when will be released, I think it is under test now.

@LinboLen
Copy link

seems that this patch is used to zone tick when use with rxjs. but the problem is that we don't want to tick such use in throttleTime, only tick in final throttleTime(5000), not with Observable.interval(100)

still wants to know what does zone rxjs patch fixed.

@LinboLen
Copy link

LinboLen commented Aug 29, 2017

I like angular/material cdk style use Rx.clain().call().call().subscribe(). only the final subscribe will cause ngZone task tick. inside of the call the internal subscribe will not cause ngZone task tick

https://github.com/JiaLiPassion/rxjs-zone/pull/1/files

@JiaLiPassion
Copy link
Collaborator

@LinboLen , in this PR, zone.js doesn't change rxjs trigger behavior, and it only ensure everybody (Observable construction, operator, subscription) can run into correct zone.
so in your code here,

  const repeater$ = Observable.of({})
      .do(() => {
        const num = Math.floor(Math.random() * 100);
        console.log('Poll action dispatched', num);
        
        this.num = num;
      })
      .delay(2000)
      .repeat();

    this.ngZone.runOutsideAngular(() => {
      this.activitySubscription = repeater$
        .subscribe(
        () => {
        },
        (error) => {
          console.error('refresh error', error);
        },
        () => console.info('finished')
      );
    });
  1. Observable construct
  2. do
  3. delay
  4. repeat

all run in ngZone, only

  1. subscribe

run outsideAngular.

So before this PR, the behavior is everything run in the subscription zone, which is not correct, (although the behavior maybe just meet your requirements).

So could you post your requirement again so I may help to find how to implement it, thanks.

@LinboLen
Copy link

LinboLen commented Aug 31, 2017

I can force deceive myself have understandd rxjs works fine with zone;😄

just search xxZone.run in rxjs.ts, it seems that if the xxZone is in right Zone. then it will fire some of magic task to run

it's works. only after 20s it will fire detectChange.

ngOnInit() {
    console.log('ngOninit');

    let repeater$;

    this.ngZone.runOutsideAngular(() => {

      repeater$ = Observable.of({})
        .do(() => {
          const num = Math.floor(Math.random() * 100);
          console.log('Poll action dispatched', num);
          this.num = num;
        })
        .map((_) => {
          return _;
        })
        .delay(2000)
        .repeat()
        .throttleTime(20000);
    });

    this.activitySubscription = repeater$
      .subscribe(
        () => {
        },
        (error) => {
          console.error('refresh error', error);
        },
        () => console.info('finished')
      );
  }

ref: https://github.com/JiaLiPassion/rxjs-zone/pull/2/files

additional proposal

so maybe can shortfy it. using rxjs in angular. and considering performance.
wrapping with

Rx.clain(ObservableXX, true/*means that this clain run out of angular*/).call(operator, arg1, arg2).call(operator, arg1, arg2)

//match in current zone just run 
Rx.clain(ObservableXX, true/*means that this clain run out of angular*/).call(operator, arg1, arg2).call(operator, arg1, arg2).subscribe()

//if not
Rx.clain(ObservableXX, true/*means that this clain run out of angular*/).call(operator, arg1, arg2).call(operator, arg1, arg2).call('subscribe', arg1, arg2)

so we could simplfy no need to introduce in ngZone in our directive/component. and it also suit for material style


if without this proposal, should guid us how to use this rxjs patch. and also improve the performance with reducing detectchange tick


bug: #890

@vitto-moz
Copy link

vitto-moz commented Mar 20, 2018

hey, everyone!
I faced an issue of losing zone during using observable operators. Below is my code before fix:

  @Effect()
  fetchEvents = this.actions$
    .ofType(EventsActionTypes.FETCH_EVENTS)
    .switchMap((action: FetchEvents): Observable<any[]> =>
      fromPromise(this.calendarService.fetchUpcomingEvents(action.id))
    ) //-------------->  here we went outside the zone due to an out of angular zone call to google api
    .map((events = []) => {
      console.log('in zone ', NgZone.isInAngularZone());  //--------------> in zone FALSE
      return events.map(event => ( new CalendarEvent(event)))
    })
    .switchMap((events: ICalendarEvent[]) => ([new FetchEventsSuccess(), new AddAll(events)]))

Next solution with an "enterZone" operator works for me

import "@ngrx/core/add/operator/enterZone"
 // -----> importing "enterZone" operator from @ngrx/core

  @Injectable()
   export class EventsEffects {
  constructor(public actions$: Actions,
              public calendarService: CalendarService,
              private zone: NgZone) {  // ------> injecting "Angular zone"
  }

  @Effect()
  fetchEvents = this.actions$
    .ofType(EventsActionTypes.FETCH_EVENTS)
    .switchMap((action: FetchEvents): Observable<any[]> =>
      fromPromise(this.calendarService.fetchUpcomingEvents(action.id))
    ) // --------------->here we went outside the zone due to an out of angular zone call to google api
    .enterZone(this.zone) // -----------> back to the Angular zone !    
     .map((events = []) => {
      console.log('in zone ', NgZone.isInAngularZone());  //--------------> in zone TRUE
      return events.map(event => ( new CalendarEvent(event)))
    })
    .switchMap((events: ICalendarEvent[]) => ([new FetchEventsSuccess(), new AddAll(events)]))

For those who don't use "ngrx/core" - here is a link to the library that contains a solution
https://github.com/ksachdeva/rxjs-zone-operators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants