-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
feat(): Angularfire auth guards #2016
Conversation
@davideast feel free to start poking at this. Going to work on the test coverage and documentation in the meantime. Targeting 5.2 release. |
FYI this has been released for testing on |
wow, so convinent :D thank you so much |
Assigning @davideast for API review. @jhuleatt if you could give a quick code review. |
FYI tests and docs need more work. |
|
||
it('should be injectable', () => { | ||
expect(router).toBeTruthy(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here's a StackOverflow answer with suggestions on ways to test auth guards. But maybe that's overkill and we just have a dummy user object and quick happy path tests for the different built-in pipes for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also spy on the AngularFireAuthGuard pipe to make sure it calls the pipe provided to it with the data provided to it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'll have to put together a proper E2E for this, something I'm working on on another branch. Think I'll punt on building more robust tests for now & take it as an action item.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of stronger names for these, but honestly I couldn't come up with something that much better. LGTM.
Any idea when there will be a new release with the merged PR? |
@svzi you can give this a try now with the beta 4 release. I'm hoping to release 5.2 stable early next week. |
@jamesdaniels Thanks a lot! But it seems like I'm missing something, or there is something not working right now. I can't compile it (based on the provided examples):
|
@svzi which version of typescript / Ng? |
@jamesdaniels Angular 7.2.0 (CLI 7.3.9) and TypeScript 3.2.2. |
I'll look into this. Will track here #2086 |
Thanks a lot, appreciate your support! |
Checklist
yarn install
,yarn test
run successfully? (yes/no; required)Description
Allow developers to protect routes in their Angular application using Firebase authentication.
TODOs
Code sample
Basic example
Use our pre-built pipes for common tests
Increase readability with our
canActivate
helperCompose your own pipes