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

ensure correct parsing the query params with '=' #24

Closed
wants to merge 1 commit into from
Closed

ensure correct parsing the query params with '=' #24

wants to merge 1 commit into from

Conversation

jhpoelen
Copy link

@jhpoelen jhpoelen commented Jun 1, 2015

Expected:
http://someurl/?param=http%3A%2F%2Fsomeurl%3Fid%3D2837
should be parsed to:
{ param: 'http://someurl?id=2837' }
actual:
{ param: 'http://someurl?id' }

This change fixes that issue.

Expected:
http://someurl/?param=http%3A%2F%2Fsomeurl%3Fid%3D2837
should be parsed to:
{ param: 'http://someurl?id=2837' }
actual:
{ param: 'http://someurl?id' }

This change fixes that issue.
@sindresorhus
Copy link
Owner

No idea what your "fix" is doing, but it's not correct.

The reason it doesn't work is that supplying full URLs isn't supported yet: #15

@jhpoelen
Copy link
Author

jhpoelen commented Jun 1, 2015

@sindresorhus Thanks for your quick response. I believe the issue is not related to the full url parsing: my example was a bit confusing... I would expect this test to pass:

it('query strings params including embedded `=`', function () {
    assert.deepEqual(qs.parse('?param=http%3A%2F%2Fsomeurl%3Fid%3D2837'), {param: 'http://someurl?id=2837'});
});

The actual result is { 'param': 'http://someurl?id' }.

I hope this clarifies the suggested fix.

sindresorhus added a commit that referenced this pull request Jun 1, 2015
@sindresorhus
Copy link
Owner

Your test seems to pass fine: https://travis-ci.org/sindresorhus/query-string/builds/64986110

Are you sure this is the query string lib you're using and you're using the latest version?

@jhpoelen
Copy link
Author

jhpoelen commented Jun 1, 2015

@sindresorhus thanks for responding and adding the test. The issue was on my end - I wasn't encoding the url properly in the href attribute. I owe you a beer ; )

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

2 participants