-
Notifications
You must be signed in to change notification settings - Fork 26.2k
feat(router): Cascading route support #16416
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
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
I signed it! (CLA) |
CLAs look good, thanks! |
FYI ci/circleci is failing the LINT on the commit message but that is for someone else's commit that's already on angular/4.0.x (INVALID COMMIT MSG: "release: cut the 4.0.1 release"). |
@vicb do you have any commentary on this since you've been involved quite a bit in the router? on a separate note while I was in there I noticed that exceptions were used during the normal paths of execution vs architecting to return a complex object or using an observable/promise reject/error. I'm up for tackling that re-factoring if you guys are on board once this request is merged. |
Super excited for this as I am converting a legacy application and initially thought this was how Guards worked so I am currently in a bind. |
@the-destro if you get a chance can you pull this into your project and verify it works as you expect for your particular use-case? If not, I'd like to catch any specific conditions you're addressing while this pull request is pending merge. Here is what you run to get this pre-merged package running: npm uninstall @angular/router --save That will run this build to test then anytime you run "npm install" after it will just revert to the main branch or you can re-run uninstall and install without specifying the separate registry. |
@matthewerwin upon quick inspection it looks like it works exactly as I need! To give further context to my route setup: http://stackoverflow.com/questions/43919064/finely-specify-routes-in-angular-2 |
As I understood from the description, this can be used insted of #14515 right? |
Is there an ETA on when this can be merged/released? |
@vicb what's the word? Anything I can do to get this merged in more quickly? |
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.
Thanks for this PR and this is a great feature. Sorry for the delay on getting back to you.
I've added a couple comments. There's also been changes requiring a rebase in order to merge this. Please go ahead and address these, then we can get this merged.
0cc8cc4
to
0bcd592
Compare
@jasonaden I squashed the commits and rebased against 4.0.x for this pull request. I also fixed the errors (The 4.0.x branch on angular errors on ./build.sh due to some things that appeared to be refactored/fixed in 4.1.x). I figured everyone would prefer 4.0.x builds without issues when pulling that branch. Separately I've also created pull requests for 4.2.x and 4.3.x and all of them use the purely rxjs style flow instead of the recursive function call that was in the initial review. That should make comparisons between the existing guard code and the cascading flow guard code very easy to identify. I cleaned up everything you mentioned in the Code Review & with the change to maintain the recognize( ) signature that also reduced the total changes in the pull request. :-) |
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.
You probably saw the note in one of the other PRs, but take a look at the new comment about mergeMapTo
and a piece of code that should fix it.
Then let's get this rebased against master
branch and the PR should also be against master
as that's where all merging happens (we create the 4.x
versions by cherry picking the appropriate commits from master
).
Hopefully you could do that today as I would like to go over this PR with someone else tomorrow and see if we can get you a final review.
@jasonaden I'll make it happen today. Will issue the new PR against master and test out if the suggested change works. |
0bcd592
to
47715d3
Compare
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.
Just a few style suggestions. FYI, @matthewerwin, #35566 on the router doc is hopefully close to landing. Thanks for updating the doc too!
Edit:
Reviewed-for: global-docs-approvers
81f50f9
to
e365b36
Compare
Added cascading routes capability to the router, added new test cases. Previous behavior resulted in dead routes (e.g. direct navigation to a route with canActivate=>false resulted in blank page). Additionally this feature improvement provides conditional routing based on params and other information that could be used to dynamically enable/disable a route while ensuring the user always falls through to a valid route. This PR passes all latest tests and should be backwards compatible.
e365b36
to
40dd2a4
Compare
@atscott and @IgorMinar - what are we going to do about this PR? It is now very stale and old. Perhaps it is not something we will ever integrate? Or perhaps it can be updated and merged? Either way it can't get into 12.0.x, so it would now be targeting 12.1.0 or later. |
I’m happy to get it rebased in for latest. I feel like every time the internal team gets ready to merge it though there is some distraction. We are also very busy on our side to keep going through the process to prepare to merge only to have it fall off priority for v10, v11 or whatever Just need someone to champion it on the Angular team to help us make sure it goes all the way through to save us all a lot of time. Let me know who I can work with and we will get this rolled up this week. |
Currently we have two main types of guards: `CanLoad`: decides if we can load a module (used with lazy loading) `CanActivate` and friends. It decides if we can activate/deactivate a route. So we always decide where we want to navigate first ("recognize") and create a new router state snapshot. And only then we run guards to check if the navigation should be allowed. This doesn't handle one very important use case where we want to decide where to navigate based on some data (e.g., who the user is). I suggest to add a new guard that allows us to do that. ``` [ {path: 'home', component: AdminHomePage, canUse: [IsAdmin]}, {path: 'home', component: SimpleHomePage} ] ``` Here, navigating to '/home' will render `AdminHomePage` if the user is an admin and will render 'SimpleHomePage' otherwise. Note that the url will remain '/home'. With the introduction of standalone components and new features in the Router such as `loadComponent`, there's a case for deprecating `CanLoad` and replacing it with the `CanMatch` guard. There are a few reasons for this: * One of the intentions of having separate providers on a Route is that lazy loading should not be an architectural feature of an application. It's an optimization you do for code size. That is, there should not be an architectural feature in the router to specifically control whether to lazy load something or not based on conditions such as authentication. This is a slight nuanced difference between the proposed canUse guard: this guard would control whether you can use the route at all and as a side-effect, whether we download the code. `CanLoad` only specified whether the code should be downloaded so canUse is more powerful and more appropriate. * The naming of `CanLoad` will be potentially misunderstood for the `loadComponent` feature. Because it applies to `loadChildren`, it feels reasonable to think that it will also apply to `loadComponent`. This isn’t the case: since we don't need to load the component until right before activation, we defer the loading until all guards/resolvers have run. When considering the removal of `CanLoad` and replacing it with `CanMatch`, this does inform another decision that needed to be made: whether it makes sense for `CanMatch` guards to return a UrlTree or if they should be restricted to just boolean. The original thought was that no, these new guards should not allow returning UrlTree because that significantly expands the intent of the feature from simply “can I use the route” to “can I use this route, and if not, should I redirect?” I now believe it should allowed to return `UrlTree` for several reasons: * For feature parity with `CanLoad` * Because whether we allow it as a return value or not, developers will still be able to trigger a redirect from the guards using the `Router.navigate` function. * Inevitably, there will be developers who disagree with the philosophical decision to disallow `UrlTree` and we don’t necessarily have a compelling reason to refuse this as a feature. Relates to angular#16211 - `CanMatch` instead of `CanActivate` would prevent blank screen. Additional work is required to close this issue. This can be accomplished by making the initial navigation result trackable (including the redirects). Resolves angular#14515 Replaces angular#16416 Resolves angular#34231 Resolves angular#17145 Resolves angular#12088
Currently we have two main types of guards: `CanLoad`: decides if we can load a module (used with lazy loading) `CanActivate` and friends. It decides if we can activate/deactivate a route. So we always decide where we want to navigate first ("recognize") and create a new router state snapshot. And only then we run guards to check if the navigation should be allowed. This doesn't handle one very important use case where we want to decide where to navigate based on some data (e.g., who the user is). I suggest to add a new guard that allows us to do that. ``` [ {path: 'home', component: AdminHomePage, canUse: [IsAdmin]}, {path: 'home', component: SimpleHomePage} ] ``` Here, navigating to '/home' will render `AdminHomePage` if the user is an admin and will render 'SimpleHomePage' otherwise. Note that the url will remain '/home'. With the introduction of standalone components and new features in the Router such as `loadComponent`, there's a case for deprecating `CanLoad` and replacing it with the `CanMatch` guard. There are a few reasons for this: * One of the intentions of having separate providers on a Route is that lazy loading should not be an architectural feature of an application. It's an optimization you do for code size. That is, there should not be an architectural feature in the router to specifically control whether to lazy load something or not based on conditions such as authentication. This is a slight nuanced difference between the proposed canUse guard: this guard would control whether you can use the route at all and as a side-effect, whether we download the code. `CanLoad` only specified whether the code should be downloaded so canUse is more powerful and more appropriate. * The naming of `CanLoad` will be potentially misunderstood for the `loadComponent` feature. Because it applies to `loadChildren`, it feels reasonable to think that it will also apply to `loadComponent`. This isn’t the case: since we don't need to load the component until right before activation, we defer the loading until all guards/resolvers have run. When considering the removal of `CanLoad` and replacing it with `CanMatch`, this does inform another decision that needed to be made: whether it makes sense for `CanMatch` guards to return a UrlTree or if they should be restricted to just boolean. The original thought was that no, these new guards should not allow returning UrlTree because that significantly expands the intent of the feature from simply “can I use the route” to “can I use this route, and if not, should I redirect?” I now believe it should allowed to return `UrlTree` for several reasons: * For feature parity with `CanLoad` * Because whether we allow it as a return value or not, developers will still be able to trigger a redirect from the guards using the `Router.navigate` function. * Inevitably, there will be developers who disagree with the philosophical decision to disallow `UrlTree` and we don’t necessarily have a compelling reason to refuse this as a feature. Relates to angular#16211 - `CanMatch` instead of `CanActivate` would prevent blank screen. Additional work is required to close this issue. This can be accomplished by making the initial navigation result trackable (including the redirects). Resolves angular#14515 Replaces angular#16416 Resolves angular#34231 Resolves angular#17145 Resolves angular#12088
Currently we have two main types of guards: `CanLoad`: decides if we can load a module (used with lazy loading) `CanActivate` and friends. It decides if we can activate/deactivate a route. So we always decide where we want to navigate first ("recognize") and create a new router state snapshot. And only then we run guards to check if the navigation should be allowed. This doesn't handle one very important use case where we want to decide where to navigate based on some data (e.g., who the user is). I suggest to add a new guard that allows us to do that. ``` [ {path: 'home', component: AdminHomePage, canUse: [IsAdmin]}, {path: 'home', component: SimpleHomePage} ] ``` Here, navigating to '/home' will render `AdminHomePage` if the user is an admin and will render 'SimpleHomePage' otherwise. Note that the url will remain '/home'. With the introduction of standalone components and new features in the Router such as `loadComponent`, there's a case for deprecating `CanLoad` and replacing it with the `CanMatch` guard. There are a few reasons for this: * One of the intentions of having separate providers on a Route is that lazy loading should not be an architectural feature of an application. It's an optimization you do for code size. That is, there should not be an architectural feature in the router to specifically control whether to lazy load something or not based on conditions such as authentication. This is a slight nuanced difference between the proposed canUse guard: this guard would control whether you can use the route at all and as a side-effect, whether we download the code. `CanLoad` only specified whether the code should be downloaded so canUse is more powerful and more appropriate. * The naming of `CanLoad` will be potentially misunderstood for the `loadComponent` feature. Because it applies to `loadChildren`, it feels reasonable to think that it will also apply to `loadComponent`. This isn’t the case: since we don't need to load the component until right before activation, we defer the loading until all guards/resolvers have run. When considering the removal of `CanLoad` and replacing it with `CanMatch`, this does inform another decision that needed to be made: whether it makes sense for `CanMatch` guards to return a UrlTree or if they should be restricted to just boolean. The original thought was that no, these new guards should not allow returning UrlTree because that significantly expands the intent of the feature from simply “can I use the route” to “can I use this route, and if not, should I redirect?” I now believe it should allowed to return `UrlTree` for several reasons: * For feature parity with `CanLoad` * Because whether we allow it as a return value or not, developers will still be able to trigger a redirect from the guards using the `Router.navigate` function. * Inevitably, there will be developers who disagree with the philosophical decision to disallow `UrlTree` and we don’t necessarily have a compelling reason to refuse this as a feature. Relates to angular#16211 - `CanMatch` instead of `CanActivate` would prevent blank screen. Additional work is required to close this issue. This can be accomplished by making the initial navigation result trackable (including the redirects). Resolves angular#14515 Replaces angular#16416 Resolves angular#34231 Resolves angular#17145 Resolves angular#12088
Currently we have two main types of guards: `CanLoad`: decides if we can load a module (used with lazy loading) `CanActivate` and friends. It decides if we can activate/deactivate a route. So we always decide where we want to navigate first ("recognize") and create a new router state snapshot. And only then we run guards to check if the navigation should be allowed. This doesn't handle one very important use case where we want to decide where to navigate based on some data (e.g., who the user is). I suggest to add a new guard that allows us to do that. ``` [ {path: 'home', component: AdminHomePage, canUse: [IsAdmin]}, {path: 'home', component: SimpleHomePage} ] ``` Here, navigating to '/home' will render `AdminHomePage` if the user is an admin and will render 'SimpleHomePage' otherwise. Note that the url will remain '/home'. With the introduction of standalone components and new features in the Router such as `loadComponent`, there's a case for deprecating `CanLoad` and replacing it with the `CanMatch` guard. There are a few reasons for this: * One of the intentions of having separate providers on a Route is that lazy loading should not be an architectural feature of an application. It's an optimization you do for code size. That is, there should not be an architectural feature in the router to specifically control whether to lazy load something or not based on conditions such as authentication. This is a slight nuanced difference between the proposed canUse guard: this guard would control whether you can use the route at all and as a side-effect, whether we download the code. `CanLoad` only specified whether the code should be downloaded so canUse is more powerful and more appropriate. * The naming of `CanLoad` will be potentially misunderstood for the `loadComponent` feature. Because it applies to `loadChildren`, it feels reasonable to think that it will also apply to `loadComponent`. This isn’t the case: since we don't need to load the component until right before activation, we defer the loading until all guards/resolvers have run. When considering the removal of `CanLoad` and replacing it with `CanMatch`, this does inform another decision that needed to be made: whether it makes sense for `CanMatch` guards to return a UrlTree or if they should be restricted to just boolean. The original thought was that no, these new guards should not allow returning UrlTree because that significantly expands the intent of the feature from simply “can I use the route” to “can I use this route, and if not, should I redirect?” I now believe it should allowed to return `UrlTree` for several reasons: * For feature parity with `CanLoad` * Because whether we allow it as a return value or not, developers will still be able to trigger a redirect from the guards using the `Router.navigate` function. * Inevitably, there will be developers who disagree with the philosophical decision to disallow `UrlTree` and we don’t necessarily have a compelling reason to refuse this as a feature. Relates to angular#16211 - `CanMatch` instead of `CanActivate` would prevent blank screen. Additional work is required to close this issue. This can be accomplished by making the initial navigation result trackable (including the redirects). Resolves angular#14515 Replaces angular#16416 Resolves angular#34231 Resolves angular#17145 Resolves angular#12088
Currently we have two main types of guards: `CanLoad`: decides if we can load a module (used with lazy loading) `CanActivate` and friends. It decides if we can activate/deactivate a route. So we always decide where we want to navigate first ("recognize") and create a new router state snapshot. And only then we run guards to check if the navigation should be allowed. This doesn't handle one very important use case where we want to decide where to navigate based on some data (e.g., who the user is). I suggest to add a new guard that allows us to do that. ``` [ {path: 'home', component: AdminHomePage, canUse: [IsAdmin]}, {path: 'home', component: SimpleHomePage} ] ``` Here, navigating to '/home' will render `AdminHomePage` if the user is an admin and will render 'SimpleHomePage' otherwise. Note that the url will remain '/home'. With the introduction of standalone components and new features in the Router such as `loadComponent`, there's a case for deprecating `CanLoad` and replacing it with the `CanMatch` guard. There are a few reasons for this: * One of the intentions of having separate providers on a Route is that lazy loading should not be an architectural feature of an application. It's an optimization you do for code size. That is, there should not be an architectural feature in the router to specifically control whether to lazy load something or not based on conditions such as authentication. This is a slight nuanced difference between the proposed canUse guard: this guard would control whether you can use the route at all and as a side-effect, whether we download the code. `CanLoad` only specified whether the code should be downloaded so canUse is more powerful and more appropriate. * The naming of `CanLoad` will be potentially misunderstood for the `loadComponent` feature. Because it applies to `loadChildren`, it feels reasonable to think that it will also apply to `loadComponent`. This isn’t the case: since we don't need to load the component until right before activation, we defer the loading until all guards/resolvers have run. When considering the removal of `CanLoad` and replacing it with `CanMatch`, this does inform another decision that needed to be made: whether it makes sense for `CanMatch` guards to return a UrlTree or if they should be restricted to just boolean. The original thought was that no, these new guards should not allow returning UrlTree because that significantly expands the intent of the feature from simply “can I use the route” to “can I use this route, and if not, should I redirect?” I now believe it should allowed to return `UrlTree` for several reasons: * For feature parity with `CanLoad` * Because whether we allow it as a return value or not, developers will still be able to trigger a redirect from the guards using the `Router.navigate` function. * Inevitably, there will be developers who disagree with the philosophical decision to disallow `UrlTree` and we don’t necessarily have a compelling reason to refuse this as a feature. Relates to angular#16211 - `CanMatch` instead of `CanActivate` would prevent blank screen. Additional work is required to close this issue. This can be accomplished by making the initial navigation result trackable (including the redirects). Resolves angular#14515 Replaces angular#16416 Resolves angular#34231 Resolves angular#17145 Resolves angular#12088
Currently we have two main types of guards: `CanLoad`: decides if we can load a module (used with lazy loading) `CanActivate` and friends. It decides if we can activate/deactivate a route. So we always decide where we want to navigate first ("recognize") and create a new router state snapshot. And only then we run guards to check if the navigation should be allowed. This doesn't handle one very important use case where we want to decide where to navigate based on some data (e.g., who the user is). I suggest to add a new guard that allows us to do that. ``` [ {path: 'home', component: AdminHomePage, canUse: [IsAdmin]}, {path: 'home', component: SimpleHomePage} ] ``` Here, navigating to '/home' will render `AdminHomePage` if the user is an admin and will render 'SimpleHomePage' otherwise. Note that the url will remain '/home'. With the introduction of standalone components and new features in the Router such as `loadComponent`, there's a case for deprecating `CanLoad` and replacing it with the `CanMatch` guard. There are a few reasons for this: * One of the intentions of having separate providers on a Route is that lazy loading should not be an architectural feature of an application. It's an optimization you do for code size. That is, there should not be an architectural feature in the router to specifically control whether to lazy load something or not based on conditions such as authentication. This is a slight nuanced difference between the proposed canUse guard: this guard would control whether you can use the route at all and as a side-effect, whether we download the code. `CanLoad` only specified whether the code should be downloaded so canUse is more powerful and more appropriate. * The naming of `CanLoad` will be potentially misunderstood for the `loadComponent` feature. Because it applies to `loadChildren`, it feels reasonable to think that it will also apply to `loadComponent`. This isn’t the case: since we don't need to load the component until right before activation, we defer the loading until all guards/resolvers have run. When considering the removal of `CanLoad` and replacing it with `CanMatch`, this does inform another decision that needed to be made: whether it makes sense for `CanMatch` guards to return a UrlTree or if they should be restricted to just boolean. The original thought was that no, these new guards should not allow returning UrlTree because that significantly expands the intent of the feature from simply “can I use the route” to “can I use this route, and if not, should I redirect?” I now believe it should allowed to return `UrlTree` for several reasons: * For feature parity with `CanLoad` * Because whether we allow it as a return value or not, developers will still be able to trigger a redirect from the guards using the `Router.navigate` function. * Inevitably, there will be developers who disagree with the philosophical decision to disallow `UrlTree` and we don’t necessarily have a compelling reason to refuse this as a feature. Relates to angular#16211 - `CanMatch` instead of `CanActivate` would prevent blank screen. Additional work is required to close this issue. This can be accomplished by making the initial navigation result trackable (including the redirects). Resolves angular#14515 Replaces angular#16416 Resolves angular#34231 Resolves angular#17145 Resolves angular#12088
Currently we have two main types of guards: `CanLoad`: decides if we can load a module (used with lazy loading) `CanActivate` and friends. It decides if we can activate/deactivate a route. So we always decide where we want to navigate first ("recognize") and create a new router state snapshot. And only then we run guards to check if the navigation should be allowed. This doesn't handle one very important use case where we want to decide where to navigate based on some data (e.g., who the user is). I suggest to add a new guard that allows us to do that. ``` [ {path: 'home', component: AdminHomePage, canUse: [IsAdmin]}, {path: 'home', component: SimpleHomePage} ] ``` Here, navigating to '/home' will render `AdminHomePage` if the user is an admin and will render 'SimpleHomePage' otherwise. Note that the url will remain '/home'. With the introduction of standalone components and new features in the Router such as `loadComponent`, there's a case for deprecating `CanLoad` and replacing it with the `CanMatch` guard. There are a few reasons for this: * One of the intentions of having separate providers on a Route is that lazy loading should not be an architectural feature of an application. It's an optimization you do for code size. That is, there should not be an architectural feature in the router to specifically control whether to lazy load something or not based on conditions such as authentication. This is a slight nuanced difference between the proposed canUse guard: this guard would control whether you can use the route at all and as a side-effect, whether we download the code. `CanLoad` only specified whether the code should be downloaded so canUse is more powerful and more appropriate. * The naming of `CanLoad` will be potentially misunderstood for the `loadComponent` feature. Because it applies to `loadChildren`, it feels reasonable to think that it will also apply to `loadComponent`. This isn’t the case: since we don't need to load the component until right before activation, we defer the loading until all guards/resolvers have run. When considering the removal of `CanLoad` and replacing it with `CanMatch`, this does inform another decision that needed to be made: whether it makes sense for `CanMatch` guards to return a UrlTree or if they should be restricted to just boolean. The original thought was that no, these new guards should not allow returning UrlTree because that significantly expands the intent of the feature from simply “can I use the route” to “can I use this route, and if not, should I redirect?” I now believe it should allowed to return `UrlTree` for several reasons: * For feature parity with `CanLoad` * Because whether we allow it as a return value or not, developers will still be able to trigger a redirect from the guards using the `Router.navigate` function. * Inevitably, there will be developers who disagree with the philosophical decision to disallow `UrlTree` and we don’t necessarily have a compelling reason to refuse this as a feature. Relates to angular#16211 - `CanMatch` instead of `CanActivate` would prevent blank screen. Additional work is required to close this issue. This can be accomplished by making the initial navigation result trackable (including the redirects). Resolves angular#14515 Replaces angular#16416 Resolves angular#34231 Resolves angular#17145 Resolves angular#12088
Currently we have two main types of guards: `CanLoad`: decides if we can load a module (used with lazy loading) `CanActivate` and friends. It decides if we can activate/deactivate a route. So we always decide where we want to navigate first ("recognize") and create a new router state snapshot. And only then we run guards to check if the navigation should be allowed. This doesn't handle one very important use case where we want to decide where to navigate based on some data (e.g., who the user is). I suggest to add a new guard that allows us to do that. ``` [ {path: 'home', component: AdminHomePage, canUse: [IsAdmin]}, {path: 'home', component: SimpleHomePage} ] ``` Here, navigating to '/home' will render `AdminHomePage` if the user is an admin and will render 'SimpleHomePage' otherwise. Note that the url will remain '/home'. With the introduction of standalone components and new features in the Router such as `loadComponent`, there's a case for deprecating `CanLoad` and replacing it with the `CanMatch` guard. There are a few reasons for this: * One of the intentions of having separate providers on a Route is that lazy loading should not be an architectural feature of an application. It's an optimization you do for code size. That is, there should not be an architectural feature in the router to specifically control whether to lazy load something or not based on conditions such as authentication. This is a slight nuanced difference between the proposed canUse guard: this guard would control whether you can use the route at all and as a side-effect, whether we download the code. `CanLoad` only specified whether the code should be downloaded so canUse is more powerful and more appropriate. * The naming of `CanLoad` will be potentially misunderstood for the `loadComponent` feature. Because it applies to `loadChildren`, it feels reasonable to think that it will also apply to `loadComponent`. This isn’t the case: since we don't need to load the component until right before activation, we defer the loading until all guards/resolvers have run. When considering the removal of `CanLoad` and replacing it with `CanMatch`, this does inform another decision that needed to be made: whether it makes sense for `CanMatch` guards to return a UrlTree or if they should be restricted to just boolean. The original thought was that no, these new guards should not allow returning UrlTree because that significantly expands the intent of the feature from simply “can I use the route” to “can I use this route, and if not, should I redirect?” I now believe it should allowed to return `UrlTree` for several reasons: * For feature parity with `CanLoad` * Because whether we allow it as a return value or not, developers will still be able to trigger a redirect from the guards using the `Router.navigate` function. * Inevitably, there will be developers who disagree with the philosophical decision to disallow `UrlTree` and we don’t necessarily have a compelling reason to refuse this as a feature. Relates to angular#16211 - `CanMatch` instead of `CanActivate` would prevent blank screen. Additional work is required to close this issue. This can be accomplished by making the initial navigation result trackable (including the redirects). Resolves angular#14515 Replaces angular#16416 Resolves angular#34231 Resolves angular#17145 Resolves angular#12088
Currently we have two main types of guards: `CanLoad`: decides if we can load a module (used with lazy loading) `CanActivate` and friends. It decides if we can activate/deactivate a route. So we always decide where we want to navigate first ("recognize") and create a new router state snapshot. And only then we run guards to check if the navigation should be allowed. This doesn't handle one very important use case where we want to decide where to navigate based on some data (e.g., who the user is). I suggest to add a new guard that allows us to do that. ``` [ {path: 'home', component: AdminHomePage, canUse: [IsAdmin]}, {path: 'home', component: SimpleHomePage} ] ``` Here, navigating to '/home' will render `AdminHomePage` if the user is an admin and will render 'SimpleHomePage' otherwise. Note that the url will remain '/home'. With the introduction of standalone components and new features in the Router such as `loadComponent`, there's a case for deprecating `CanLoad` and replacing it with the `CanMatch` guard. There are a few reasons for this: * One of the intentions of having separate providers on a Route is that lazy loading should not be an architectural feature of an application. It's an optimization you do for code size. That is, there should not be an architectural feature in the router to specifically control whether to lazy load something or not based on conditions such as authentication. This is a slight nuanced difference between the proposed canUse guard: this guard would control whether you can use the route at all and as a side-effect, whether we download the code. `CanLoad` only specified whether the code should be downloaded so canUse is more powerful and more appropriate. * The naming of `CanLoad` will be potentially misunderstood for the `loadComponent` feature. Because it applies to `loadChildren`, it feels reasonable to think that it will also apply to `loadComponent`. This isn’t the case: since we don't need to load the component until right before activation, we defer the loading until all guards/resolvers have run. When considering the removal of `CanLoad` and replacing it with `CanMatch`, this does inform another decision that needed to be made: whether it makes sense for `CanMatch` guards to return a UrlTree or if they should be restricted to just boolean. The original thought was that no, these new guards should not allow returning UrlTree because that significantly expands the intent of the feature from simply “can I use the route” to “can I use this route, and if not, should I redirect?” I now believe it should allowed to return `UrlTree` for several reasons: * For feature parity with `CanLoad` * Because whether we allow it as a return value or not, developers will still be able to trigger a redirect from the guards using the `Router.navigate` function. * Inevitably, there will be developers who disagree with the philosophical decision to disallow `UrlTree` and we don’t necessarily have a compelling reason to refuse this as a feature. Relates to angular#16211 - `CanMatch` instead of `CanActivate` would prevent blank screen. Additional work is required to close this issue. This can be accomplished by making the initial navigation result trackable (including the redirects). Resolves angular#14515 Replaces angular#16416 Resolves angular#34231 Resolves angular#17145 Resolves angular#12088
…tch (#46021) Currently we have two main types of guards: `CanLoad`: decides if we can load a module (used with lazy loading) `CanActivate` and friends. It decides if we can activate/deactivate a route. So we always decide where we want to navigate first ("recognize") and create a new router state snapshot. And only then we run guards to check if the navigation should be allowed. This doesn't handle one very important use case where we want to decide where to navigate based on some data (e.g., who the user is). I suggest to add a new guard that allows us to do that. ``` [ {path: 'home', component: AdminHomePage, canUse: [IsAdmin]}, {path: 'home', component: SimpleHomePage} ] ``` Here, navigating to '/home' will render `AdminHomePage` if the user is an admin and will render 'SimpleHomePage' otherwise. Note that the url will remain '/home'. With the introduction of standalone components and new features in the Router such as `loadComponent`, there's a case for deprecating `CanLoad` and replacing it with the `CanMatch` guard. There are a few reasons for this: * One of the intentions of having separate providers on a Route is that lazy loading should not be an architectural feature of an application. It's an optimization you do for code size. That is, there should not be an architectural feature in the router to specifically control whether to lazy load something or not based on conditions such as authentication. This is a slight nuanced difference between the proposed canUse guard: this guard would control whether you can use the route at all and as a side-effect, whether we download the code. `CanLoad` only specified whether the code should be downloaded so canUse is more powerful and more appropriate. * The naming of `CanLoad` will be potentially misunderstood for the `loadComponent` feature. Because it applies to `loadChildren`, it feels reasonable to think that it will also apply to `loadComponent`. This isn’t the case: since we don't need to load the component until right before activation, we defer the loading until all guards/resolvers have run. When considering the removal of `CanLoad` and replacing it with `CanMatch`, this does inform another decision that needed to be made: whether it makes sense for `CanMatch` guards to return a UrlTree or if they should be restricted to just boolean. The original thought was that no, these new guards should not allow returning UrlTree because that significantly expands the intent of the feature from simply “can I use the route” to “can I use this route, and if not, should I redirect?” I now believe it should allowed to return `UrlTree` for several reasons: * For feature parity with `CanLoad` * Because whether we allow it as a return value or not, developers will still be able to trigger a redirect from the guards using the `Router.navigate` function. * Inevitably, there will be developers who disagree with the philosophical decision to disallow `UrlTree` and we don’t necessarily have a compelling reason to refuse this as a feature. Relates to #16211 - `CanMatch` instead of `CanActivate` would prevent blank screen. Additional work is required to close this issue. This can be accomplished by making the initial navigation result trackable (including the redirects). Resolves #14515 Replaces #16416 Resolves #34231 Resolves #17145 Resolves #12088 PR Close #46021
We recently landed the While the implementation we went with in the end differed from the approach taken here, the attention and feedback from this PR was a major factor in driving discussions and ultimately the solution to these use-cases. |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
If you want the awesomely cool capability of doing things like yoursite.com/@<user|org> in your routes in Angular and having the name be an organization or an individual handled by separate modules (a la Twitter-style or Medium-style) you should definitely upvote this PR and help us get it merged!
This PR accomplishes:
Added cascading routes capability to the router, added new test cases.
Previous behavior resulted in dead routes (e.g. direct navigation to a
route with canActivate=>false resulted in blank page). Additionally this
feature improvement provides conditional routing based on params and other
information that could be used to dynamically enable/disable a route while
ensuring the user always falls through to a valid route.
Fall-through on routing tables instead of having routes themselves redirect is a well-established manner for handling permission-based routing (i.e. guard-based). This PR has been out since Angular 2.0 and is still not merged -- please help us get this done so it isn't an ongoing maintenance task to update it with every version change.
This PR passes all latest tests and should be backwards compatible.
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (check one with "x")
What is the current behavior? (You can also link to an open issue here)
#16211
Current behavior results in a dead link/route when routes are inaccessible due to canActivate rejecting the promise or returning false.
Router attempts to recover state to originating route state when canActivate or other preactivation guards fail (e.g. data loading guard). This either leaves the user clicking a link that does nothing, a blank page if the URL is hit directly (bookmark, external site link, page refresh while canActivate changes since original load).
What is the new behavior?
New behavior falls through to other valid routes including the default '**' route to give the user a 404 or other message. This allows more sophisticated routing scenarios based on conditionality as well as elegant handling for no-access scenarios that do not redirect.
Does this PR introduce a breaking change? (check one with "x")
If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...
Other information: