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

querystring result not consistent with encodeURIComponent #5309

Closed
jacobp100 opened this issue Feb 18, 2016 · 3 comments
Closed

querystring result not consistent with encodeURIComponent #5309

jacobp100 opened this issue Feb 18, 2016 · 3 comments
Labels
querystring Issues and PRs related to the built-in querystring module.

Comments

@jacobp100
Copy link

QueryString has a comment that states the following,

   // replaces encodeURIComponent
   // http://www.ecma-international.org/ecma-262/5.1/#sec-15.1.3.4

However, the spec states that you should use ToString. In the code, you’re using ValueOf. I.e.

const x = { valueOf: function() { return 'other result'; }, toString: function() { return 'spec result'; } }
encodeURIComponent(x) !== require('querystring').escape(x)
@Fishrock123 Fishrock123 added the querystring Issues and PRs related to the built-in querystring module. label Feb 18, 2016
@silentroach
Copy link
Contributor

first comment is about querystring.escape and it really replaces encodeURIComponent the way you expect.

> require('querystring').escape({test: 5, toString: () => 10})
'10'

if you mean querystring.stringify - it is not a replacement to encodeURIComponent.

@jacobp100
Copy link
Author

No, I am talking about querystring.escape. If you take the object I gave you and run it through both encodeURIComponent and querystring.escape, you will get different results.

@silentroach
Copy link
Contributor

got that, sorry. lets try to fix it :)

jasnell pushed a commit that referenced this issue Apr 26, 2016
This commit fixes an inconsistency in querystring.escape objects handling
compared to native encodeURIComponent function.

Fixes: #5309
PR-URL: #5341
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
querystring Issues and PRs related to the built-in querystring module.
Projects
None yet
Development

No branches or pull requests

3 participants