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

Change multiple values to use array format #10

Closed
wants to merge 2 commits into from
Closed

Change multiple values to use array format #10

wants to merge 2 commits into from

Conversation

eduardoboucas
Copy link

When the object to be encoded contains an array, the resulting string simply repeats the key for each of the values.

Example:

{foo: ['bar', 'baz', 'qux']}

Results in:

foo=bar&foo=baz&foo=qux

However, this only gives access to one of the values (typically the last one). I came across this issue when trying to use oauth to generate a OAuth URL for the GitHub API.
The solution was to use the array convention to represent multiple values for the same key.

Example:

foo[]=bar&foo[]=baz&foo[]=qux

I don't know if you want to adopt this convention (as it can potentially break existing implementations), but in any case I've included the change in the PR.

EDIT (25 July): It doesn't make sense to just change the convention for everyone that's already using the package, so I've added an optional parameter to encode() that activates the array convention. It defaults to false, so everything should work exactly as is if the parameter isn't activated.

To activate it, arrayMode must be set to true. Because of the existing optional parameters in the function, it's necessary to actively set them to undefined in the function call — unfortunate, but in my opinion still better than not having this option at all.

Example:

querystring.encode({foo: ['bar', 'baz', 'qux']}, undefined, undefined, undefined, true);

@emilbayes
Copy link

Hey @eduardoboucas, this is pretty common, maybe you should make the same suggestion to nodejs/node? I don't see it suggested over there before. It might be out of scope for the built-in query string implementation though

@johannesnagl
Copy link

is this PR going to happen? We need exactly the same behavior and I was wondering why this is not part of the library :/

@sontek
Copy link

sontek commented Aug 23, 2016

This project hasn't accepted changes in 4 years, so probably dead? Maybe someone can fork it and get this in?

@sontek
Copy link

sontek commented Aug 23, 2016

https://www.npmjs.com/package/qs seems to support it

@RusAlex
Copy link

RusAlex commented Sep 5, 2016

Interesting. I also met the issue. Will try to use qs

@medikoo
Copy link
Collaborator

medikoo commented Apr 11, 2017

If you're interested to bring this module back to living, here's the proposed roadmap: #20 PR's are very welcome (and I will handle them promptly, I promise :)

I'm going to close this issue, as if request is on pair with Node.js implementation then it'll be there. If it's not, then we're not interesting in deriving from API as it's in Node.js

@medikoo medikoo closed this Apr 11, 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.

6 participants