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

Remove ampersand #23

Closed
wants to merge 8 commits into from
Closed

Conversation

saromanov
Copy link
Contributor

Hi!
If query string starts with &, result from parse will be { '': null, data: 'item' }

console.log(query.parse("&data=item")); //{ '': null, data: 'item' }

So, I've just a made more predictable behavior.

console.log(query.parse("&data=item")); //{ data: 'item' }

@sindresorhus
Copy link
Owner

That's a invalid query string. The ampersand & is a separator. Why do you need this?

@@ -11,6 +11,10 @@ exports.parse = function (str) {
return {};
}

if (str[0] == '&') {
str = str.substr(1, str.length);
}
Copy link
Owner

Choose a reason for hiding this comment

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

Should implemented on line 8.

Copy link
Owner

Choose a reason for hiding this comment

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

@saromanov ⬆️

@saromanov
Copy link
Contributor Author

This is remove only from start of the string. Not from all of this. For example:

console.log(query.parse("&data=foo&data=bar&fun=bag)); //{ data: [ 'foo', 'bar' ], fun: 'bag' }

@sindresorhus
Copy link
Owner

No, I mean, why does your querystring start with a &? As I said, that's invalid, some I'm curious why it's like that.

@saromanov
Copy link
Contributor Author

Yes, i know that this is invalid, but my main motivation was replace incorrect behavior(in my point of view) with ampersand. In other cases, even if you put something incorrect, your response will be also invalid

console.log(query.parse("/data=foo"));  //{ '/data': 'foo' }
console.log(query.parse("*data=foo"));  //{ '*data': 'foo' }

No problems. But with the ampersand case, you'll get:

console.log(query.parse("&data=foo")); //{ '': null, data: 'foo' }

And i decided to make "clear" response {data: 'foo'} without {'': null}.

@sindresorhus
Copy link
Owner

Alright, I guess it wouldn't hurt. Just fix the inline comments and I'll merge.

@saromanov
Copy link
Contributor Author

Fixed!

@@ -5,6 +5,10 @@ exports.parse = function (str) {
return {};
}

if (str[0] == '&') {

Choose a reason for hiding this comment

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

triple equal

@saromanov
Copy link
Contributor Author

Thanks, @arthurvr! Fixed, again.

@sindresorhus
Copy link
Owner

@saromanov ➡️ #23 (comment)

@saromanov
Copy link
Contributor Author

@sindresorhus , sorry, i not have much experience with github pull requests and i don't know what to do next. I was fixed with your comments(move to the right lines), but should i open new pull request? Or commit it again?

@sindresorhus
Copy link
Owner

Sorry if I weren't clear. The & should be stripped here https://github.com/sindresorhus/query-string/pull/23/files#diff-168726dbe96b3ce427e7fedce31bb0bcR12 as we already strip # and ? there.

@saromanov
Copy link
Contributor Author

Now, its clear for me :)

@sindresorhus
Copy link
Owner

Looks good. Thanks :)

index.js Outdated
}

str = str.trim().replace(/^(\?|#)/, '');
str = str.trim().replace(/^(\?|#|&)/, '');

Choose a reason for hiding this comment

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

It is simpler and faster to use a regex /^[?#&]/ instead

Copy link
Owner

Choose a reason for hiding this comment

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

I doubt it has any practical performance difference, but it is indeed simpler. Thanks :)

f283341

sindresorhus added a commit that referenced this pull request Jul 24, 2017
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

4 participants