repeat any query params after a fragment #1341
Merged
Conversation
This file contains 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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Url.generate was modified in 2015 to put any query params before any fragment element of the url, based on pull #214 which suggested that the URI RFC says fragment should be at the end and referencing a possible issue with an iOS app not being able to read query params after the fragment. In fact the RFC specifically says "The characters slash ("/") and question mark ("?") are allowed to represent data within the fragment identifier." - https://tools.ietf.org/html/rfc3986#section-3.5 .
Putting the query params before the hash fragment breaks the reset password flow for Angular apps using HashLocationStrategy, i.e. the redirect to the Angular reset password component will have a hash fragment in it (see #599 ). At this stage, 4 years later, I can imagine the maintainers of this lib will be wary of reverting to putting the params after the fragment. At the same time it feels like overkill to introduce a config element just for this (and the complexity of doing that is probably why we have no PR 3 years after the Angular issue was first raised). So what I am proposing is simply a belt and braces approach - put the query params both before and after the fragment. I have confirmed this fixes the reset password flow for Angular when using HashLocationStrategy and https://github.com/neroniaky/angular-token. I have also confirmed that this doesn't break the currently working flow for Angular apps using the default PathLocationStrategy. I've also checked the resulting rediect url with the repeated params for validity against various URL parsers and it passes.
What do you think?