Skip to content

Redirect if a guard returns a UrlTree #26521

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.

Sign up for GitHub

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

Jump to bottom
Closed
jasonaden wants to merge 7 commits into from
Closed

Redirect if a guard returns a UrlTree #26521

jasonaden wants to merge 7 commits into from

Conversation

@jasonaden
Copy link
Contributor

@jasonaden jasonaden commented Oct 17, 2018 •
edited
Loading

Fixes #24618

@jasonaden jasonaden requested a review from benlesh October 17, 2018 16:52
@mary-poppins
Copy link

mary-poppins commented Oct 17, 2018

You can preview ef48f54 at https://pr26521-ef48f54.ngbuilds.io/.

benlesh
benlesh suggested changes Oct 17, 2018
View reviewed changes
Copy link
Contributor

@benlesh benlesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only one small change I'd make.

packages/router/test/router.spec.ts Outdated
Copy link
Contributor

@benlesh benlesh Oct 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RxJS core team recommends using an Observer for all subscriptions that use more than one callback, like:

obs.subscribe({
  next(value) { },
  error(err) { }
});

Copy link
Member

@gkalpak gkalpak Oct 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OOC, why?

Copy link
Contributor Author

@jasonaden jasonaden Oct 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Convention and more readable.

JoostK
JoostK requested changes Oct 17, 2018
View reviewed changes
@mary-poppins
Copy link

mary-poppins commented Oct 17, 2018

You can preview a1f41fc at https://pr26521-a1f41fc.ngbuilds.io/.

@jasonaden jasonaden force-pushed the FW-153_url_returned_from_guards branch from a1f41fc to 94328f9 Compare October 17, 2018 22:14
@mary-poppins
Copy link

mary-poppins commented Oct 17, 2018

You can preview 94328f9 at https://pr26521-94328f9.ngbuilds.io/.

@jasonaden
refactor(router): add type definitions for guard functions
8021972
@jasonaden
refactor(router): remove type casting from wrapIntoObservable private…
4069aae 
… function
@jasonaden
feat(router): allow redirect from guards by returning UrlTree
b5c615d 
* Improve type checking within the CheckGuards function
* Change public API to allow returning of UrlTree instead of boolean
@jasonaden
feat(router): allow guards to return UrlTree as well as boolean
2510420 
* Removed `andObservable` helper function in favor of inline implementation
* Flow `boolean | UrlTree` through guards check
* Add tests to verify behavior of `checkGuards` function flowing `UrlTree` properly
@jasonaden
feat(router): guard returning UrlTree cancels current navigation and …
1cb4a3b 
…redirects

Fixes angular#24618
FW-153 #resolve
@jasonaden jasonaden force-pushed the FW-153_url_returned_from_guards branch from 94328f9 to 1cb4a3b Compare October 17, 2018 22:21
@jasonaden
Copy link
Contributor Author

jasonaden commented Oct 17, 2018

Presubmit

@jasonaden jasonaden requested a review from IgorMinar October 18, 2018 00:33
@jasonaden
Copy link
Contributor Author

jasonaden commented Oct 18, 2018

Global Presubmit

sarunint
sarunint reviewed Oct 18, 2018
View reviewed changes
Copy link
Contributor

@sarunint sarunint left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great feature!

@mary-poppins
Copy link

mary-poppins commented Oct 30, 2018

You can preview eeeb56b at https://pr26521-eeeb56b.ngbuilds.io/.

@jasonaden jasonaden force-pushed the FW-153_url_returned_from_guards branch from eeeb56b to 43e34dc Compare October 30, 2018 19:26
@mary-poppins
Copy link

mary-poppins commented Oct 30, 2018

You can preview 43e34dc at https://pr26521-43e34dc.ngbuilds.io/.

@jasonaden jasonaden force-pushed the FW-153_url_returned_from_guards branch from 43e34dc to 43e505e Compare October 30, 2018 19:47
@jasonaden jasonaden added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release area: router labels Oct 31, 2018
@ngbot ngbot bot added this to the needsTriage milestone Oct 31, 2018
@jasonaden jasonaden added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Oct 31, 2018
@matsko matsko closed this in 9f4299d Oct 31, 2018
matsko pushed a commit that referenced this pull request Oct 31, 2018
@jasonaden @matsko
refactor(router): add type definitions for guard functions (#26521)
6712492 
PR Close #26521
matsko pushed a commit that referenced this pull request Oct 31, 2018
@jasonaden @matsko
refactor(router): remove type casting from wrapIntoObservable private…
17586f1 
… function (#26521)

PR Close #26521
matsko pushed a commit that referenced this pull request Oct 31, 2018
@jasonaden @matsko
feat(router): allow redirect from guards by returning UrlTree (#26521)
152ca66 
* Improve type checking within the CheckGuards function
* Change public API to allow returning of UrlTree instead of boolean

PR Close #26521
matsko pushed a commit that referenced this pull request Oct 31, 2018
@jasonaden @matsko
feat(router): allow guards to return UrlTree as well as boolean (#26521)
081f95c 
* Removed `andObservable` helper function in favor of inline implementation
* Flow `boolean | UrlTree` through guards check
* Add tests to verify behavior of `checkGuards` function flowing `UrlTree` properly

PR Close #26521
matsko pushed a commit that referenced this pull request Oct 31, 2018
@jasonaden @matsko
feat(router): guard returning UrlTree cancels current navigation and …
4e9f2e5 
…redirects (#26521)

Fixes #24618
FW-153 #resolve

PR Close #26521
@Airblader Airblader mentioned this pull request Nov 17, 2018
Closed
@jasonaden jasonaden mentioned this pull request Apr 4, 2019
Closed
@angular-automatic-lock-bot
Copy link

angular-automatic-lock-bot bot commented Sep 14, 2019

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@JoostK JoostK JoostK requested changes

@IgorMinar IgorMinar Awaiting requested review from IgorMinar

+4 more reviewers

@sarunint sarunint sarunint left review comments

@myrddraall myrddraall myrddraall left review comments

@gkalpak gkalpak gkalpak left review comments

@benlesh benlesh benlesh approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker area: router cla: yes merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: major This PR is targeted for the next major release

Projects

None yet

Milestone

needsTriage

Development

Successfully merging this pull request may close these issues.

feature(router): Provide a way to coalesce multiple failed guards so only a single redirect is called

8 participants

@jasonaden @mary-poppins @JoostK @benlesh @sarunint @myrddraall @gkalpak @googlebot