-
Notifications
You must be signed in to change notification settings - Fork 25.5k
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
fix(router): linking to redirects #3624
Conversation
c5672bf
to
e8e1b35
Compare
LGTM. |
Did we talk about what we do if a redirect results in another redirect? Or does this code recurse over itself if the follow-up instruction is a redirect? |
Redirects should be final, not transitive. I don't want to allow cascading redirects within components like this:
But I think a case where each child in a chain expands the URL or does redirects would be fine. I'll add another test case for that scenario. |
e8e1b35
to
d32ee07
Compare
d32ee07
to
05d706e
Compare
This should be good now, assuming CI goes green. @matsko – I added another test for multiple redirects, and pulled some code out into a private method: https://github.com/angular/angular/pull/3624/files#diff-56a3f902acf026b93362cffea30dcf53R63 Can you take a look again? |
sent to presubmit 😄 |
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. |
I pulled out the logic for finding a maximal element in a list into a facade, and parameterized it so you can provide your own mapping from element to value.
I've described pitfalls of this approach here: #3335 (comment)
Nevertheless, I think this is the best solution for now. I think we'll have to revisit redirects again to cover more general cases.