-
Notifications
You must be signed in to change notification settings - Fork 27k
Add CanMatch guard to Route #46021
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
Add CanMatch guard to Route #46021
Conversation
857c619 to
c366391
Compare
jessicajaniuk
left a comment
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.
LGTM 🍪
reviewed-for: public-api, fw-core
c366391 to
d977ccc
Compare
2249eb0 to
feaf664
Compare
dylhunn
left a comment
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.
reviewed-for: public-api
alxhub
left a comment
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.
Reviewed-for: size-tracking
…unctions This commit moves the runCanLoadGuards to the same location as other guard execution functions
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
…Promise` The `Observable` chain is currenlty the most straightforward way to handle navigation cancellations where we ensure that the cancelled navigation does not continue to be processed. Until we design and implement an alternative way to accomplish equivalent functionality, we need to maintain the `Observable` chain wherever we might execute user code. One reason for this isthat user code may contain redirects so we do not want to execute those redirects if the navigation was already cancelled.
feaf664 to
5ec921b
Compare
|
This PR was merged into the repository by commit 72e6a94. |
…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
…Promise` (#46021) The `Observable` chain is currenlty the most straightforward way to handle navigation cancellations where we ensure that the cancelled navigation does not continue to be processed. Until we design and implement an alternative way to accomplish equivalent functionality, we need to maintain the `Observable` chain wherever we might execute user code. One reason for this isthat user code may contain redirects so we do not want to execute those redirects if the navigation was already cancelled. PR Close #46021
|
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. |
Currently we have two main types of guards:
CanLoad: decides if we can load a module (used with lazy loading)CanActivateand 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.
Here, navigating to '/home' will render
AdminHomePageif 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
CanLoadand replacing it with theCanMatchguard. There are a few reasons for this: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.
CanLoadonly specified whether the code should be downloaded so canUse is more powerful and more appropriate.CanLoadwill be potentially misunderstood for theloadComponentfeature.Because it applies to
loadChildren, it feels reasonable to think that it willalso apply to
loadComponent. This isn’t the case: since we don't needto load the component until right before activation, we defer the
loading until all guards/resolvers have run.
When considering the removal of
CanLoadand replacing it withCanMatch, thisdoes inform another decision that needed to be made: whether it makes sense for
CanMatchguards 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
UrlTreefor several reasons:CanLoadable to trigger a redirect from the guards using the
Router.navigatefunction.to disallow
UrlTreeand we don’t necessarily have a compelling reason to refuse this as a feature.Relates to #16211 -
CanMatchinstead ofCanActivatewould preventblank 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