Skip to content

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

Closed
wants to merge 5 commits into from

Conversation

atscott
Copy link
Contributor

@atscott atscott commented May 17, 2022

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

@atscott atscott added the target: minor This PR is targeted for the next minor release label May 17, 2022
@ngbot ngbot bot added this to the Backlog milestone May 17, 2022
@atscott atscott changed the title Can use observable recognize Add CanMatch guard to Route May 17, 2022
@atscott atscott force-pushed the canUseObservableRecognize branch 5 times, most recently from 857c619 to c366391 Compare May 17, 2022 23:09
@atscott
Copy link
Contributor Author

atscott commented May 18, 2022

presubmit

@atscott atscott marked this pull request as ready for review May 18, 2022 21:51
@pullapprove pullapprove bot requested review from alxhub, dylhunn and jessicajaniuk May 18, 2022 21:51
Copy link
Contributor

@jessicajaniuk jessicajaniuk left a 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

@atscott atscott force-pushed the canUseObservableRecognize branch from c366391 to d977ccc Compare May 18, 2022 23:37
@atscott atscott force-pushed the canUseObservableRecognize branch from 2249eb0 to feaf664 Compare June 1, 2022 23:00
Copy link
Contributor

@dylhunn dylhunn left a 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

Copy link
Member

@alxhub alxhub left a 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

atscott added 5 commits June 10, 2022 16:27
…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.
@atscott atscott force-pushed the canUseObservableRecognize branch from feaf664 to 5ec921b Compare June 10, 2022 23:28
@atscott atscott added the action: global presubmit The PR is in need of a google3 global presubmit label Jun 13, 2022
@atscott
Copy link
Contributor Author

atscott commented Jun 13, 2022

TGP

@atscott atscott removed the action: global presubmit The PR is in need of a google3 global presubmit label Jun 13, 2022
@atscott atscott removed the request for review from pkozlowski-opensource June 13, 2022 22:38
@atscott atscott added the action: merge The PR is ready for merge by the caretaker label Jun 13, 2022
@jessicajaniuk
Copy link
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
@angular-automatic-lock-bot
Copy link

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 Jul 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: router target: minor This PR is targeted for the next minor release
Projects
None yet
4 participants