tj / node-querystring

querystring parser for node and the browser - supporting nesting (used by Express, Connect, etc)
MIT License
455 stars 66 forks source link

stringify: Drop Array index, when possible #71

Open kenperkins opened 10 years ago

kenperkins commented 10 years ago

I noticed that the following happens on an array:

var qs = require('qs');

qs.stringify({ ids: [1,2,3,4,5,6] });

Output:

ids[0]=1&ids[1]=2&ids[2]=3&ids[3]=4&ids[4]=5&ids[5]=6

Is there any way to stringify without the index location? i.e.:

ids=1&ids=2&ids=3&ids=4&ids=5&ids=6
kenperkins commented 10 years ago

Running this command yields a different output than input:

qs.stringify(qs.parse('id=1&id=2'));

output:

'id[0]=1&id[1]=2'
tj commented 10 years ago

yeah +1 on fixing that, it should definitely not result in different output haha

buschtoens commented 10 years ago

This behaviour makes sense, as arrays can contain empty (undefined) slots.

var arr = [0, 1, 2, 3, 4, 5];
delete arr[3];
console.log(arr); // [0, 1, 2, undefined, 4, 5]

If we'd now blindly convert arrays we end up with arr=0&arr=1&arr=2&arr=4&arr=5, which has wrong indexing, as it translates back to [0, 1, 2, 4, 5]. The empty slot is missing.

However, you have a valid point. We should always try to produce the shortest possible but valid output we can. So we can still implement this, if we check the arrays for consistency first.

kenperkins commented 10 years ago

The problem is both arr=0 and arr[0] may be valid output. The latter matters if order is critical.

This bug was to try and figure out how to accommodate both output formats.

mxk1235 commented 10 years ago

we are currently blocked by this. can I get an update on a workaround?

more specifically, we would like to have

require('qs').stringify( { 'key[]' : [ 1, 2 ] } ); 'key%5B%5D=1&key%5B%5D=2'

what's the correct way of doing it?

kenperkins commented 10 years ago

Thanks for refreshing this issue @mxk1235. The issue here is that currently qs.stringify is opinionated for how it serializes an array. And it shouldn't be. Perhaps the default should stay as is (to not break back-compat) but it should be perfectly allowable to serialized without index locations.

mxk1235 commented 10 years ago

any idea on the timeframe for fixing this?

myndzi commented 10 years ago

I also have a need for repeating indexes due to the structure of an API I need to use. The 'request' module uses qs to create its query strings, so I tracked it back to here. Specifically, it would be extremely helpful to have the following be possible:

qs.stringify({ key: ['val1', 'val2', 'val3', ...] });

outputs:

key=val1&key=val2&key=val3...
jkarttunen commented 9 years ago

+1 for fixing this

pietia commented 9 years ago

We've managed to fix this issue by monkey patching Request:

var querystring = require('querystring');
request.Request.prototype.form = function (form) {
  if (form) {
    this.setHeader('content-type', 'application/x-www-form-urlencoded; charset=utf-8')
    this.body = querystring.stringify(form).toString('utf8');
    return this
  }
  // create form-data object
  this._form = new FormData();
  return this._form;
};
rlidwka commented 9 years ago

@jkarttunen , @pietia , this repository is moved to https://github.com/hapijs/qs . If you want this implemented, please ask there instead.

kenperkins commented 9 years ago

Uh. why do the deprecation this way instead of via github move repo?

rlidwka commented 9 years ago

No idea, you might find some answers here: https://github.com/visionmedia/node-querystring/issues/113