Closed
Conversation
857c619 to
c366391
Compare
Contributor
Author
jessicajaniuk
approved these changes
May 18, 2022
Contributor
jessicajaniuk
left a comment
There was a problem hiding this comment.
LGTM 🍪
reviewed-for: public-api, fw-core
c366391 to
d977ccc
Compare
2249eb0 to
feaf664
Compare
dylhunn
approved these changes
Jun 2, 2022
Contributor
dylhunn
left a comment
There was a problem hiding this comment.
reviewed-for: public-api
…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
Contributor
Author
Contributor
|
This PR was merged into the repository by commit 72e6a94. |
jessicajaniuk
pushed a commit
that referenced
this pull request
Jun 13, 2022
…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
jessicajaniuk
pushed a commit
that referenced
this pull request
Jun 13, 2022
…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
3 tasks
|
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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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