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

ApplicationRef.isStable never goes to "true" when an instance of AngularFireAuth is injected #1347

Closed
pglazkov opened this Issue Nov 19, 2017 · 22 comments

Comments

Projects
None yet
@pglazkov

pglazkov commented Nov 19, 2017

When AngularFireAuth is injected anywhere on the page, the ApplicationRef.isStable never emits true. This causes problems with latest @angular/service-worker, which relies on ApplicationRef.isStable to install the worker. This probably also would cause server-side rendering to timeout, but I didn't test this.

Version info

Angular: 5.0.0

Firebase: 4.6.2

AngularFire: 5.0.0-rc4

How to reproduce these conditions

Inject AngularFireAuth into a component and subscribe to the ApplicationRef.isStable observable:

import { Component, ApplicationRef, ChangeDetectorRef } from '@angular/core';
import { AngularFireAuth } from 'angularfire2/auth';

@Component({
  selector: 'my-app',
  templateUrl: './app.component.html',
  styleUrls: [ './app.component.css' ]
})
export class AppComponent  {
  constructor(appRef: ApplicationRef, af: AngularFireAuth) {    
    appRef.isStable.subscribe(s => {
      console.log(s); 
    });
  }
}

Here is a project that reproduces the problem: https://stackblitz.com/edit/angularfire2-appref-isstable

Expected behavior

The ApplicationRef.isStable observable should emit true shortly after application is loaded.

Actual behavior

The ApplicationRef.isStable observable emits only initial false value.

@jamesdaniels

This comment has been minimized.

Member

jamesdaniels commented Nov 19, 2017

I'm actually tackling right now. Thanks for the report!

@jamesdaniels

This comment has been minimized.

Member

jamesdaniels commented Nov 19, 2017

Once patched I'll cut a new RC.

@playground

This comment has been minimized.

playground commented Nov 27, 2017

I have a somewhat different issue, seems like return ApplicationRef; never got executed.

I'm new to angularfire2, can someone offer some good use cases for using angularfire2?

eggp added a commit to minutuslausus/jegybazar that referenced this issue Nov 29, 2017

@deftomat

This comment has been minimized.

deftomat commented Dec 11, 2017

The same issue when I injects AngularFirestore.
We cannot use an @angular/service-worker as it is not registered due to this issue.

@LanderBeeuwsaert

This comment has been minimized.

LanderBeeuwsaert commented Dec 14, 2017

jep, same issue here.

@val-samonte

This comment has been minimized.

val-samonte commented Dec 27, 2017

I am having the same issue when injecting AngularFirestore

@patrickmichalina

This comment has been minimized.

patrickmichalina commented Jan 14, 2018

Any updates on this issue?

@Splaktar

This comment has been minimized.

Member

Splaktar commented Jan 28, 2018

I am seeing this as well. It is being tracked in angular/angular#20970 where I added a full reproduction repo on GitHub. It's possible that angular/zone.js#999 may solve this, but it's still waiting on a review.

Update: I tested the PR (angular/zone.js#999) but it didn't help in my case.

@Splaktar

This comment has been minimized.

Member

Splaktar commented Feb 10, 2018

From @JiaLiPassion "firebase auth make a very very long setTimeout to do proactive token refresh. (more than 200000ms)". Based on this comment, AngularFire2 needs to be doing something like this when running this long setTimeout.

I'm not sure how we could disable Firebase Auth from doing this so that AngularFire2 could do it in an Angular-friendly way, but this will be a problem for integration testing as well as Service Worker registration. @jamesdaniels is anyone on the Firebase team able to look into this?

For anyone blocked on the Service Worker registration, you can manually call navigator.serviceWorker.register('/ngsw-worker.js') yourself as mentioned here.

@ArtDesire

This comment has been minimized.

ArtDesire commented Feb 11, 2018

angularfire2 cannot leave RC stage until this is fixed.

@jamesdaniels

This comment has been minimized.

Member

jamesdaniels commented May 4, 2018

Fixed in rc7

@PierreDuc

This comment has been minimized.

PierreDuc commented May 28, 2018

The issue persists in the Auth module.

After you login some setTimeout is set at 3300000ms. The code is too obfuscated to pinpoint where it exactly comes from, but it is somewhere inside auth.js from firebase.

By using a so called TaskTrackingZoneSpec, I was able to determine that when this zone task is created, the NgZone.isInAngularZone() returns true, and the e2e tests will timeout.

When I put this inside the onScheduleTask of the trackingZone:

if (task.type === 'macroTask' && task.data.delay > 3200000 && task.data.delay < 3400000) {
  setTimeout(() => {
    Zone.current.cancelTask(task);
  });
}`

The tests will run fine, because that particular task is no longer blocking the stable state of the angular zone, but also that task no longer exists, and I don't know how important it is ;).

Apparently some calls inside the AngularfireAuthModule trigger this timeout to run inside the zone. I haven't figured out which one exactly yet. My best guess is that it has something to do with the IdToken, judging by the stack trace

@PierreDuc

This comment has been minimized.

PierreDuc commented May 29, 2018

After a little more digging I found that the problem is originating from the fireauth.AuthUser.prototype.initializeProactiveRefreshUtility_.

This is at least triggered on login with using just email and password, or by using the getIdToken() and setting forceRefresh to true.

For now the workaround is whenever you call these functions is to wrap it inside a ngZone.runOutsideAngular -> ngZone.run

@PierreDuc

This comment has been minimized.

PierreDuc commented May 31, 2018

Just talking with myself here, I managed to work around it by having to override the AngularFireAuth like this:

{ provide: AngularFireAuth, useClass: AngularFireOwnAuth },

And have this:

@Injectable()
class AngularFireOwnAuth extends AngularFireAuth {
  readonly idToken: Observable<string | null>;

  constructor(
     @Inject(FirebaseOptionsToken) options: any,
     @Optional() @Inject(FirebaseNameOrConfigToken) nameOrConfig: string|any|undefined,
     @Inject(PLATFORM_ID) platformId: Object,
     zone: NgZone
  ) {
    super(options, nameOrConfig, platformId, zone);

    this.idToken = this.user.pipe(
      switchMap(async user =>
        user ? zone.run(async() => await zone.runOutsideAngular(() => user.getIdToken())) : null
      )
    );
  }
}

This will make sure the getIdToken is called outside the angular zone. If this is called inside the zone, it will trigger the long setTimeout and prevent angular from ever getting stable. At least not for 3300000ms :)

@epelc

This comment has been minimized.

epelc commented May 31, 2018

Thanks @PierreDuc! I can confirm the run outside zone method works as intended. I'm not using angular fire but it fixes it with interval from rxjs since it's the same idea.

@jamesdaniels

This comment has been minimized.

Member

jamesdaniels commented May 31, 2018

@PierreDuc

This comment has been minimized.

PierreDuc commented Jun 1, 2018

@jamesdaniels The problem is that the observable this.user is inside the zone. So any pipe from it will be in the zone as well. Besides having to override the service, I also had to make any call to the AngularFireAuth, which in turn can possibly trigger the initializeProactiveRefreshUtility_ from firebase, to run outside the zone, and reenter after done.

I didn't have the time to figure out which of the calls trigger it, but I'm sure that at least user.getIdToken is a naughty one

@JiaLiPassion

This comment has been minimized.

Contributor

JiaLiPassion commented Jun 1, 2018

@PierreDuc, maybe the problem is goog.Promise is not zone aware? Could you provide a reproduce repo?

@PierreDuc

This comment has been minimized.

PierreDuc commented Jun 1, 2018

@JiaLiPassion well, the issue is a long lasting setTimeout, triggered by firebase itself. This timeout is running inside the angular zone, and preventing the zone to be without macroTasks. Don't think that goog.Promise is involved here.

To reproduce a repo, I have to set-up a mock firebase app, where you can register and login. Don't think I have time soon to make something like that

@JiaLiPassion

This comment has been minimized.

Contributor

JiaLiPassion commented Jun 1, 2018

@PierreDuc , ok, I know about the long lasting setTimeout one.
About the code here

if (task.type === 'macroTask' && task.data.delay > 3200000 && task.data.delay < 3400000) {
  setTimeout(() => {
    Zone.current.cancelTask(task);
  });
}`

Maybe there is a better way to solve this one, if this long setTimeout is not relevant to test,
you can try this in onScheduleTask,

onScheduleTask: (delegate, curr, target, task) => {
  if ( task.source === 'setTimeout' && task.data.delay > 3200000 && task.data.delay < 3400000) {
      task.cancelScheduleRequest();
      task = Zone.root.scheduleTask(task);
      return task;
  }
  return delegate.scheduleTask(target, task);
}
@PierreDuc

This comment has been minimized.

PierreDuc commented Jun 1, 2018

@JiaLiPassion Ah, but that's just in an additional test class called a TaskTrackingZoneSpec, which I modified a bit to determine what was causing the hanging angular zone.

But yes instead of overriding the AngularFireAuth, I could implement it like this, just in the e2e, although I don't really want something like this in my specs. It will make the e2e code different from the production code.

@JiaLiPassion

This comment has been minimized.

Contributor

JiaLiPassion commented Jun 1, 2018

@PierreDuc , got it, I believe in Testability there is an additional API to let you decide when to done, but anyway, it also use TaskTrackingZoneSpec. When you have time, if you can share your e2e code, maybe I can help to find out how to make it look better, because it maybe a common use case for using zone for test. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment