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.

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

Closed
wants to merge 2 commits into from

Conversation

btford
Copy link
Contributor

@btford btford commented Aug 13, 2015

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.

@btford btford added type: bug/fix hotlist: GT action: review The PR is still awaiting reviews from at least one requested reviewer effort2: days labels Aug 13, 2015
@btford btford added this to the alpha-36 milestone Aug 13, 2015
@btford btford force-pushed the feat-router-link-to-redirects branch from c5672bf to e8e1b35 Compare August 13, 2015 18:36
@matsko
Copy link
Contributor

matsko commented Aug 13, 2015

LGTM.

@matsko
Copy link
Contributor

matsko commented Aug 13, 2015

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?

@btford
Copy link
Contributor Author

btford commented Aug 13, 2015

Redirects should be final, not transitive. I don't want to allow cascading redirects within components like this:

/foo -> /bar
/bar -> baz

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.

@btford btford force-pushed the feat-router-link-to-redirects branch from e8e1b35 to d32ee07 Compare August 17, 2015 20:07
@btford btford force-pushed the feat-router-link-to-redirects branch from d32ee07 to 05d706e Compare August 17, 2015 21:24
@btford
Copy link
Contributor Author

btford commented Aug 17, 2015

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?

@btford
Copy link
Contributor Author

btford commented Aug 19, 2015

sent to presubmit 😄

@btford btford closed this in 72e0b8f Aug 19, 2015
@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 Sep 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: review The PR is still awaiting reviews from at least one requested reviewer cla: yes effort2: days type: bug/fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants