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

updated querystring #16

Merged
merged 1 commit into from Apr 26, 2013
Merged

Conversation

juliangruber
Copy link
Member

Updated querystring to the latest version + my fixes for browsers (see tj/node-querystring#57)

@defunctzombie
Copy link
Collaborator

Can we just instead use the node-querystring module like we use http-browserify?

@medikoo
Copy link

medikoo commented Apr 23, 2013

There's also https://github.com/Gozala/querystring which is a code mirror for Node's querystring

@juliangruber
Copy link
Member Author

that module still needs some shims though

@medikoo
Copy link

medikoo commented Apr 23, 2013

what shims? what for? I use it without issues in browsers.

@juliangruber
Copy link
Member Author

It uses Object.keys, Array.isArray and more. So it's an issue if you need support for IE<9 without polyfills.

@medikoo
Copy link

medikoo commented Apr 23, 2013

Yeah, if you need that, you load es5-shim before.

@juliangruber
Copy link
Member Author

Nah, no polyfills for me plz

@medikoo
Copy link

medikoo commented Apr 23, 2013

@juliangruber without taking advantage of ES5 you're just locking yourself in year 2005.

@ghost
Copy link

ghost commented Apr 23, 2013

@shtylman how about merging this now and then when somebody has a version on npm with the same fixes we can depend on that?

@juliangruber
Copy link
Member Author

my PR on qs has already been merged, it just needs to be released.

@defunctzombie
Copy link
Collaborator

Since the PR on qs has been merged, I will wait for that to be released
(hoping that won't take too long). If someone could ping this PR when that
happens I can make the changes to use that module.

On Tue, Apr 23, 2013 at 9:00 AM, Julian Gruber notifications@github.comwrote:

my PR on qs has already been merged, it just needs to be released.


Reply to this email directly or view it on GitHubhttps://github.com//pull/16#issuecomment-16856337
.

@juliangruber
Copy link
Member Author

ping ping ping :)

@defunctzombie
Copy link
Collaborator

Looking at the qs module. I don't see it being node.js compatible with the following:
http://nodejs.org/api/querystring.html#querystring_querystring_escape

Thoughts?

@juliangruber
Copy link
Member Author

Oh, true....ok then I'll have to make https://github.com/Gozala/querystring run in older browsers...

@juliangruber
Copy link
Member Author

Waiting for Gozala/querystring#4

@juliangruber
Copy link
Member Author

So, Gozala doesn't want to merge my PR, because shims shouldn't be part of the code.

Either we say browserify requires polyfills or we include my fork: https://github.com/juliangruber/querystring/tree/compat.

@defunctzombie
Copy link
Collaborator

Since the other projects seem to be going nowhere, I will just merge this. We can deal with other issues as they come up.

defunctzombie added a commit that referenced this pull request Apr 26, 2013
@defunctzombie defunctzombie merged commit 1eebfe1 into browserify:master Apr 26, 2013
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

3 participants