Inconsistencies with node core

Gozala / querystring

Node's querystring module for all engines.

MIT License

287 stars 72 forks source link

Inconsistencies with node core #6

Open feross opened 10 years ago

feross commented 10 years ago

In node 0.11, options can include a decodeURIComponent or encodeURIComponent property. See: https://github.com/joyent/node/blob/master/lib/querystring.js#L180
The encode/decode functions should be overridable via querystring.escape and querystring.unescape, see: http://nodejs.org/api/querystring.html#querystring_querystring_escape

Gozala commented 9 years ago

@medikoo is maintaining this repo, so I don't want to make API changes without him being ok with that. I think work that @feross is doing is very important & it would be a shame if do not support him, regardless of our API preferences.

feross commented 9 years ago

Thanks @Gozala :)

The goal of this module is to match the node/iojs querystring module, right? Seems like API changes shouldn't be debated in this repo, but in the node/iojs projects before API changes are made. Once node/iojs commit to an API, the module ought to just mirror it :) At least, that's what I do in buffer.

medikoo commented 9 years ago

@Gozala this can't be reliable achieved without getting back to single module version. I'm fine with this, as in my work I rely more on querystring2 package.

If you're also ok with it, in some free moment I can make it so it's single module with latest node.js implementation.

philipwalton commented 9 years ago

I just encountered this issue and completely agree with @feross.

If you're not going to match the API exactly, at a bare minimum the differences should be documented. As it is today I had to read through the code to see if my implementation was buggy or if this library just didn't support this option.

Gozala commented 9 years ago

@medikoo sure let's merge with your querystring2 if resolves this issue for everyone.

emilbayes commented 9 years ago

Any progress on this? ~~Browserify seems to rely on the module by default, so it's a shame that it's not 1:1 with Node core~~ My bad, the browserify README.md links to this module, but the package.json specifies querystring-es3

medikoo commented 7 years ago

Should be addressed with https://github.com/Gozala/querystring/issues/20 it covers also ES3 version, PR's are very welcome (and I will handle them promptly).