Skip to content
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

refactor(router): Produce error message when canActivate is used with… #40067

Closed
wants to merge 4 commits into from
Closed

refactor(router): Produce error message when canActivate is used with… #40067

wants to merge 4 commits into from

Conversation

atscott
Copy link
Contributor

@atscott atscott commented Dec 10, 2020

… redirectTo

Redirects in the router are processed before activations. This means that a canActivate will
never execute if a route has a redirect. Rather than silently ignoring
the invalid config, developers should be notified so they know why it
doesn't work.

Closes #18605
The feature request for a function/class redirect is covered in #13373.

Note that this is a new error that would prevent routing from working. We may want to discuss the need to land this along with a migration that removes canActivate from configs with redirectTo if this is found to be a common occurrence. Even without a migration, it may still be necessary to land this in a minor/major version to prevent new failures in patch version updates.

@atscott atscott added area: router target: patch This PR is targeted for the next patch release labels Dec 10, 2020
@atscott atscott requested a review from mhevery December 10, 2020 16:03
@google-cla google-cla bot added the cla: yes label Dec 10, 2020
@ngbot ngbot bot added this to the Backlog milestone Dec 10, 2020
mhevery
mhevery suggested changes Dec 10, 2020
View reviewed changes
Copy link
Contributor

@mhevery mhevery left a comment

Choose a reason for hiding this comment

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

Can we include a test?

@atscott
refactor(router): Produce error message when canActivate is used with…
8a03bde 
… redirectTo

Redirects in the router are processed before activations. This means that a canActivate will
never execute if a route has a redirect. Rather than silently ignoring
the invalid config, developers should be notified so they know why it
doesn't work.

Closes angular#18605
The feature request for a function/class redirect is covered in angular#13373.
@atscott
Copy link
Contributor Author

atscott commented Dec 10, 2020

Can we include a test?

Good call - done.

@atscott atscott requested a review from mhevery December 10, 2020 17:11
@atscott
Copy link
Contributor Author

atscott commented Dec 10, 2020

presubmit

@atscott
Copy link
Contributor Author

atscott commented Dec 10, 2020

Initial presubmit show at least two problematic configs in the subset of tests run.
One important note when making the decision on when/how to land this: This check is guarded by the ngDevMode flag so it would not cause failures in production for applications that have this flag correctly configured.

@atscott atscott added target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels Dec 15, 2020
@atscott
feat(core): Add schematic to fix invalid Route configs
4e44076 
`Route` configs with `redirectTo` as well as `canActivate` are not valid
because the `canActivate` guards will never execute. Redirects are
applied before activation. There is no error currently for these
configs, but another commit will change this so that an error does
appear in dev mode. This migration fixes the configs by removing the
`canActivate` property.
@pullapprove pullapprove bot added area: dev-infra Issues related to Angular's own dev infra (build, test, CI, releasing) area: migrations Issues related to `ng update` migrations labels Dec 15, 2020
@atscott atscott requested review from crisbeto and removed request for alxhub December 15, 2020 17:32
@atscott
Copy link
Contributor Author

atscott commented Dec 15, 2020

@mhevery - There were 5 places globally in g3 where this produced new errors. Again, these errors would only happen in dev mode. Still, I've updated the label to target a minor release.

I've also added a commit that includes a migration for this change. @crisbeto - would you mind reviewing the schematic change?

crisbeto
crisbeto approved these changes Dec 15, 2020
View reviewed changes
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM on the migration code with a few nits.

packages/core/schematics/migrations/can-activate-with-redirect-to/index.ts Outdated Show resolved Hide resolved
packages/core/schematics/migrations/can-activate-with-redirect-to/util.ts Outdated Show resolved Hide resolved
packages/router/src/utils/config.ts Show resolved Hide resolved
mhevery
mhevery approved these changes Dec 16, 2020
View reviewed changes
Copy link
Contributor

@mhevery mhevery left a comment

Choose a reason for hiding this comment

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

LGTM for router (not familiar with schematics)

@atscott atscott requested review from josephperrott and removed request for IgorMinar December 17, 2020 17:28
@atscott
Copy link
Contributor Author

atscott commented Dec 17, 2020

presubmit

josephperrott
josephperrott approved these changes Dec 17, 2020
View reviewed changes
Copy link
Member

@josephperrott josephperrott 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: dev-infra

@atscott atscott added the action: merge The PR is ready for merge by the caretaker label Dec 17, 2020
josephperrott pushed a commit that referenced this pull request Jan 5, 2021
@atscott @josephperrott
feat(core): Add schematic to fix invalid Route configs (#40067)
805b4f9 
`Route` configs with `redirectTo` as well as `canActivate` are not valid
because the `canActivate` guards will never execute. Redirects are
applied before activation. There is no error currently for these
configs, but another commit will change this so that an error does
appear in dev mode. This migration fixes the configs by removing the
`canActivate` property.

PR Close #40067
@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 Feb 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mhevery mhevery mhevery approved these changes

@crisbeto crisbeto crisbeto approved these changes

@josephperrott josephperrott josephperrott approved these changes

@marekdon marekdon marekdon approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: dev-infra Issues related to Angular's own dev infra (build, test, CI, releasing) area: migrations Issues related to `ng update` migrations area: router cla: yes target: minor This PR is targeted for the next minor release
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

CanActivate not called on redirectTo router rule; but no error either
5 participants
@atscott @mhevery @crisbeto @josephperrott @marekdon