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

Add support for question marks in hash fragments #36

Merged
merged 3 commits into from
Jul 12, 2016

Conversation

gigabo
Copy link
Contributor

@gigabo gigabo commented Jun 27, 2016

URLs with no query string, but with a hash fragment containing ? have the portion of the hash fragment prior to the ? erroneously included as part of the path.

For example, given /foo#bar?baz, the path should be /foo, but is instead /foo#bar.

From rfc3986:

The characters slash ("/") and question mark ("?") are allowed to represent data within the fragment identifier.

From [rfc3986](https://tools.ietf.org/html/rfc3986#section-3.5):

> The characters slash ("/") and question mark ("?") are allowed to
> represent data within the fragment identifier.
@yahoocla
Copy link

CLA is valid!

@mridgway mridgway added the ready label Jun 27, 2016
@coveralls
Copy link

coveralls commented Jun 27, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling ae09616 on gigabo:hash-quest into 18ff14b on yahoo:master.

@gigabo
Copy link
Contributor Author

gigabo commented Jul 5, 2016

Hi, is someone available to take a look at this? It addresses a fairly troublesome bug.

@redonkulus
Copy link
Contributor

Sorry for the late response @gigabo. Code changes look ok to me. 👍


// Leave `pos` at the beginning of the query-string, if any.
['#', '?'].forEach(function(delimiter){
pos = url.indexOf(delimiter);
Copy link
Member

@lingyan lingyan Jul 8, 2016

Choose a reason for hiding this comment

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

thanks @gigabo for the fix! would it be better to use pos = path.indexOf(delimiter)? Sorry for the delay in response, for some reason, I did not get notification of your PR :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @lingyan. Nice catch. I've pushed a patch.

@coveralls
Copy link

coveralls commented Jul 12, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling aa98701 on gigabo:hash-quest into 18ff14b on yahoo:master.

@lingyan
Copy link
Member

lingyan commented Jul 12, 2016

Great, @gigabo Once build passes, I will merge and release. Thanks a lot for your contribution!

@lingyan lingyan merged commit 24f0445 into yahoo:master Jul 12, 2016
@lingyan lingyan removed the ready label Jul 12, 2016
@lingyan
Copy link
Member

lingyan commented Jul 12, 2016

@gigabo New version 2.0.2 has been released and published to npm, with your fix: https://github.com/yahoo/routr/releases/tag/v2.0.2

@gigabo
Copy link
Contributor Author

gigabo commented Jul 12, 2016

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants