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

ApplicationRef.isStable is always false when using this package #961

Closed
BillyBlaze opened this issue Feb 8, 2021 · 6 comments
Closed

ApplicationRef.isStable is always false when using this package #961

BillyBlaze opened this issue Feb 8, 2021 · 6 comments
Assignees

Comments

@BillyBlaze
Copy link
Contributor

Describe the bug
When you kick-off the setup (with checkAuth) then this package will create intervals and timeouts. When using Service workers it's important to get the application into stable mode or else the new version isn't downloaded. However this is not possible because this package is making it unstable and it will never turn stable.

To Reproduce
Steps to reproduce the behavior:

  1. Add this snippet somewhere (e.g. constructor of app.module.ts):
     this.applicationRef.isStable.pipe(
     	tap(isStable => console.log(`ApplicationRef::stable`, isStable))
     ).subscribe();
  2. Initialize this package with calling checkAuth (we do it in the initializer, so that the guards are fully working with this package)
  3. Notice that in the console the message 'ApplicationRef::stable false' is recieved but you will never get 'ApplicationRef::stable true'
    • When you do have a "stable" version you should see 'ApplicationRef::stable false' and than followed up by 'ApplicationRef::stable true'

Expected behavior
intervals and timeouts should be run outside the NgZone so that it wont interfere with the "stability" of Angular.

This package reaches stable for me when these two culprits are run outside angular:


this.scheduledHeartBeatRunning = setTimeout(pollServerSessionRecur, this.heartBeatInterval);

Potential fixes
intervall.service.ts:

startPeriodicTokenCheck(repeatAfterSeconds) {
	const millisecondsDelayBetweenTokenCheck = repeatAfterSeconds * 1000;
	const update$ = new BehaviorSubject({});

	this.zone.runOutsideAngular(() => setInterval(() => this.zone.run(() => update$.next({})), millisecondsDelayBetweenTokenCheck))

	return update$;
}

check-session.service.ts:

this.zone.runOutsideAngular(() => {
	this.scheduledHeartBeatRunning = setTimeout(() => this.zone.run(pollServerSessionRecur), this.heartBeatInterval);
});
@FabianGosebrink
Copy link
Collaborator

Hey, thanks for the issue. Can you do a PR, then we can test it. Thanks.

@BillyBlaze
Copy link
Contributor Author

I have submitted the PR; I have adjusted the 'potential fix' a little bit to make it work nice with the already existing unit tests.

Also this should be considered a performance enhancement since ChangeDetection will now also not be called after each interval and timeout.

@FabianGosebrink
Copy link
Collaborator

Hey @BillyBlaze , thanks for this. In the mentioned issue we now have to emit from isAuthenticated$ outside the ngzone. People have to manually hook into this if they want their UI to be updated. Do you think it is possible to hook back into the ngZone without breaking the stability we fixed in this issue but that the people do not have to hook into ngZone manually? Where in the lib would be the best place without breaking the fix here? Thanks for your thoughts.

@FabianGosebrink FabianGosebrink self-assigned this Apr 30, 2021
@BillyBlaze
Copy link
Contributor Author

Hi @FabianGosebrink,

I do not work anymore at the company where I implemented the code, so it's hard for me to verify/test it. However there is one emit that is not run inside the zone:

https://github.com/damienbod/angular-auth-oidc-client/pull/965/files#diff-73284c44fab8baf773a2eb916d8abbf42bb245218348a0ef4a0a4efa7071e888R23

Hopefully this issue is fixed if we just change:
intervalId = setInterval(() => subscriber.next(), millisecondsDelayBetweenTokenCheck);

into:

intervalId = setInterval(() => this.zone.run(() => subscriber.next()), millisecondsDelayBetweenTokenCheck);

@FabianGosebrink
Copy link
Collaborator

Hey @BillyBlaze , thanks for the quick reply.

Yes, this would be my first approach as well. Thanks, I will try that! Thank you!

@FabianGosebrink
Copy link
Collaborator

Quick update. In my tests this fixes it. Thank you!

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

No branches or pull requests

2 participants