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

Defacto Standard: field[]=a&field[]=b&field[]=c becomes field=["a","b","c"] #895

Closed
jmealo opened this issue Sep 8, 2015 · 7 comments
Closed
Labels

Comments

@jmealo
Copy link

jmealo commented Sep 8, 2015

I think we should re-open #444 as the behavior requested is found in PHP, Ruby and ASP.NET as well as jQuery's .param making this a defacto standard.

For your consideration, I have submitted PR #896 for the server component and #11 for the client component.

Behavior

Input:

field[]=a
field[]=b
field[]=c

Output:

field = ["a","b","c"]

Input:

pizza[left]=pepperoni
pizza[right]=anchovies

Output:

pizza = { "left": "pepperoni", "right": "anchovies" }

Input:

pizza[left][]=pepperoni
pizza[left][]=bacon
pizza[right][]=anchovies

Output:

pizza = { "left": ["pepperoni", "bacon"], "right": ["anchovies"] }
@micahr
Copy link
Member

micahr commented Sep 8, 2015

I think this might be better implemented as a new plugin instead of changing how each plugin uses the querystring. What do you think @DonutEspresso?

@DonutEspresso
Copy link
Member

Thanks for the PR, @jmealo! There's currently ongoing work to pull the plugins out into their own repo, so I think we may hold on to this PR in the meantime, and eventually merge it into the new repo.

That said, it looks like qs should support the parsing the behavior we want, at least, based on the readme. If we can continue to use qs and get the desired behavior, that would be preferable, and opt into the new behavior using an options flag. Nothing against the other module, but I'd prefer something that has ongoing activity and is still being actively supported.

If that's not possible, we could probably roll this as a separate plugin, and let users choose which plugin they want to use. I feel the existing plugin should adhere to the standard as much as possible, even despite the syntax's defacto usage.

@Cyberuben
Copy link

Is there any progress on this issue?

@DonutEspresso
Copy link
Member

We aren't actively working on this PR - all new plugin changes should go to the plugins repo. That said, it looks like qs should already support this behavior. If that's not the case, would be happy to look into it.

@trentm
Copy link
Contributor

trentm commented Oct 25, 2016

Hi all,

I'm wondering if restify@4 and restify-clients@1 already work adequately here without needing a change:

Current restify-client@1 behaviour doesn't use the bracket notation:

> querystring.stringify({field: ['a', 'b', 'c']})
'field=a&field=b&field=c'

However current restify@4 server-side behaviour does parse that syntax
to an array:

> qs.parse('field=a&field=b&field=c')
{ field: [ 'a', 'b', 'c' ] }

And current restify@4 server-side behaviour does handle the bracket notation,
if there is a client that sends it:

> qs.parse('field[]=a&field[]=b&field[]=c')
{ field: [ 'a', 'b', 'c' ] }

Independently, I suppose a case could be made for restify-client to use the qs module instead of the current querystring to perhaps favour the bracket notation. However I don't know if that is needed for functionality. Perhaps for talking to servers other than those implemented with restify?

@jmealo
Copy link
Author

jmealo commented Oct 28, 2016

@DonutEspresso: I can't speak towards the long term support of the included module/repo but the code contained predates Node.js and should be considered mature. I believe that I used the smallest/safest code I could find to keep the size down. It was a while ago some I'm a bit fuzzy on the specifics.

I don't use restify anymore and will defer to the maintainers.

I do however stand by the work. The CI tests fail due to this being a multiple-repository pull request

@retrohacker
Copy link
Member

retrohacker commented Jul 28, 2017

Migrated to https://github.com/restify/node-restify/blob/5.x/FEATURE_REQUESTS.md

Closed restify/clients#11 leaving a comment explaining the rationale.

A note for future contributors (:heart:) a majority of the work needed to carry this over the finish line has been done by @jmealo, you can find it on their PR above 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants