Skip to content

canActivate is broken on 6.0.0 #2312

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

Closed
jimmykane opened this issue Feb 5, 2020 · 10 comments
Closed

canActivate is broken on 6.0.0 #2312

jimmykane opened this issue Feb 5, 2020 · 10 comments

Comments

@jimmykane
Copy link

Version info

Angular: 9-rc14

Firebase: 7.8.0

AngularFire: 6.0.0-rc0

Other (e.g. Ionic/Cordova, Node, browser, operating system):

How to reproduce these conditions

Failing test unit, Plunkr, or JSFiddle demonstrating the problem

Steps to set up and reproduce

Sample data and security rules

<-- include/attach/link to some json sample data (or provide credentials to a sanitized, test Firebase project) -->

Debug output

** Errors in the JavaScript console **

** Output from firebase.database().enableLogging(true); **

** Screenshots **

Expected behavior

canActivate works

Actual behavior

canActivate doesn't work

canActivate(
    next: ActivatedRouteSnapshot,
    state: RouterStateSnapshot): Observable<boolean> | Promise<boolean> | boolean {
    if (this.authenticationService.authenticated()) {
      return true;
    }

    return this.authenticationService.user.pipe(take(1)).pipe(map(user => !!user)).pipe(tap(loggedIn => {
      if (!loggedIn) {
        this.logger.warn(`Access denied`);
        this.snackBar.open('Access denied', null, {
          duration: 2000,
        });
        this.router.navigate(['/login']);
      }
    }))

  }

The above used to work.

Currently with 6.0.0-rc0 the above doesn't work

Returning true or an observable does not allow the route to work

@jimmykane
Copy link
Author

is this related to #2309 ?

@jamesdaniels
Copy link
Member

Interesting, I don't think it's related, as that bug didn't make it's way into the v6 branch. I must have caught it during the rebase.

TBH I haven't used auth-guards in the since cutting RC.0, it's entirely possible I broke something since ngcc was very unhappy with that module. I'll give it a spin later today, in the meantime LMK if you figure out anything more.

Thanks for testing the release candidate BTW!

@sarunint
Copy link
Contributor

sarunint commented Feb 6, 2020

Interesting discussions are going on #2309 and yes, I think this issue is related to that issue.

@wSedlacek
Copy link

wSedlacek commented Feb 6, 2020

I decided to give the RC a try today too.

I am noticing when using canActivate() I am getting a lot of slow down on my page.
My Angular Material text boxes will take forever to update (See Photo)
As soon as canActivate() is commented out the performance returns to normal.
When performance profiling I did at one point notice a lot of GC, but I was unable to isolate it down specifically.
Maybe the changes you just committed address this behavior?

Usage:

import {
  AngularFireAuthGuardModule,
  redirectLoggedInTo,
  canActivate,
} from '@angular/fire/auth-guard';

const redirectLoginToDashboard = () => redirectLoggedInTo(['dashboard']);

const routes: Routes = [
  {
    path: '',
    loadChildren: () => import('@story-squad/pages').then((m) => m.LoginModule),
    ...canActivate(redirectLoginToDashboard),
  },
];

Behavior:
Screen Shot 2020-02-06 at 00 43 12

Update:
I can confirm this issue does not occur of 5.4.2

@jamesdaniels
Copy link
Member

Can you give 6.0.0-rc.1 a spin? I'm cutting it now with the fix that went into 5.4.2

@jimmykane
Copy link
Author

Sure! Doing that asap

@jimmykane
Copy link
Author

@jamesdaniels yes that fixes my issues.

To be precise the canActivate on my side was using a service instance. The service onInit was getting the authState and a DB user. Once the service was instanciated the authState would not emit later (only on first call). Thus the canActivate would not return anything (router was waiting)

Should also fix @wSedlacek 's issue

@jimmykane
Copy link
Author

Feel free to close this for me

@wSedlacek
Copy link

Updated to 6.0.0-rc.1, the issue with performance still seems to remain for me.
Here are two preformance profiles I took, the fast one with 5.4.2 and the slow one on 6.0.0-rc.1
profiles.zip

@jamesdaniels
Copy link
Member

@jimmykane glad to hear we fixed it and apologies for the break. Thanks again for the feedback!

I created a new issue for us to track the performance regression @wSedlacek, thanks for the profile data.

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

4 participants