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

Stringify with comma format should add square brackets to parameter name to identity array with single value #315

Open
sanex3339 opened this issue Jun 4, 2019 · 17 comments · May be fixed by #316

Comments

@sanex3339
Copy link

sanex3339 commented Jun 4, 2019

Hi. With new comma array format, stringify of array with single value will produce following result:

const options = {
    arrayFormat: 'comma'
};
qs.stringify({foo: [1]}, options); // foo=1
qs.stringify({foo: [1, 2]}, options); // foo=1,2

So, on parse stage, parsing will produce single value.

qs.parse('foo=1); // {foo: 1}
qs.parse('foo=1,2'); // {foo: [1, 2]}

Maybe add square brackets to parameter name, when comma array format is used?

qs.stringify({foo: [1]}, options); // foo[]=1
qs.stringify({foo: [1, 2]}, options); // foo[]=1,2
qs.parse('foo[]=1); // {foo: [1]}
qs.parse('foo[]=1,2'); // {foo: [1, 2]}

Currently, parsing of the following query string foo[]=1,2 working as expected

@sanex3339 sanex3339 changed the title Stringify with comma format should add brackets to parameter name to identity array with single value Stringify with comma format should add square brackets to parameter name to identity array with single value Jun 4, 2019
@sanex3339
Copy link
Author

@daggerjames what you think?

@sanex3339
Copy link
Author

PR:
#316

@daggerjames
Copy link
Contributor

I'm a little worry about that adding square brackets would cause version break rather than minor version update with feature improvement.

A quick fix would be use another parameter to control whether or not the output of stringify should contains whether or not the brackets. But it would increment the complexity of this lib.

In fact, there is no 'official' documentation that defines how 'comma' should behave in this situation. I'm cool with brackets, and when I do that PR, I made 'comma' as explicit required as an option, as a hint that 'you know you want to handle an array, and comma is the joint symbol'.

But for now, I care more about existing code... people who upgrade with subsequence version of qs might find that it would break their code if they have seen brackets as not what they saw yesterday (and they might need to deal with it on their server code!)

@sanex3339
Copy link
Author

sanex3339 commented Jun 4, 2019

Yes, existing code is important, but firstly, notation with brackets already correctly parsed, and inconsistence between stringify of array with single item and with multiple items can cause some real errors (i got one and create this PR).

It can be major version update, yes.

@daggerjames
Copy link
Contributor

I think your concern is more on parse rather than how we stringify a list.

If Qs.parse("foo=a,b", {comma: true}) // {foo: ['a', 'b']}
and Qs.parse("foo=a, {comma: true}) // {foo: ['a']})

would it fix your case?

Also, I check the arrayFormat=repeat, and that parse can not distinguish between single value and array either. I have no idea about whether we should have a consistent result of return value as array. I personally have no bias on both sides. However, I do think if a user explicitly set 'comma' as true, he truly want an array more than a vague result.

I'm still not comfortable with add brackets as default. It would cause much confusion with the result of using arrayFormat: bracket. However, if your backend need brackets, the current workaround would be Qs.stringify({'foo[]': ['b', 'c']}, {arrayFormat: 'comma', encode: false}).

It leave the last question, how we treat parse with foo[]=a,b?

The expected answer I believe is Qs.parse("foo[]=a,b", {comma: true}) // foo: ['a', 'b']
and Qs.parse("foo[]=a", {comma: true}) // foo: ['a']. And they are the current result.

@sanex3339
Copy link
Author

he expected answer I believe is Qs.parse("foo[]=a,b", {comma: true}) // foo: ['a', 'b']
and Qs.parse("foo[]=a", {comma: true}) // foo: ['a']. And they are the current result.

This, this is expected result for me and my teammates

@ljharb
Copy link
Owner

ljharb commented Jun 5, 2019

So to sum up - the confusion is around when comma is true, and the key lacks trailing brackets?

@sanex3339
Copy link
Author

The confusion is around when comma is true, and there is no way to determine that value was array with single element, after stringify, because usually all code expect values in consistent format.

@ljharb
Copy link
Owner

ljharb commented Jun 5, 2019

Agreed - the way to convey that it's an array is to add brackets on the end of the key, as asks the OP.

This would be a breaking change at this point, but we could add another option (or make comma support both true and 'bracket'?) that enables this behavior.

@daggerjames
Copy link
Contributor

Instead of changing the default behavior of arrayFormat: 'comma',
we could add another option such as
commaSuffix: false // false | 'bracket', with false as default,

and for this issue case, an example would be
Qs.stringify({foo: 'a'}, { arrayFormat: 'comma', commaSuffix: 'bracket' }) // foo[]=a

Do I sum right?
@sanex3339 @ljharb

@ljharb
Copy link
Owner

ljharb commented Jun 6, 2019

either that, or arrayFormat: 'comma-bracket'? not sure which would be better.

@sanex3339
Copy link
Author

comma-bracket for me. Less options is better.

@thinh105
Copy link

thinh105 commented Sep 11, 2020

So, what should I do now if I want to preserve my one-element Array?

Is there any workaround?

As @daggerjames suggests, we need to add brackets on the end of the key, am I right?

Here is my repl.it,

const  qs = require('qs');


const stringifyQuery = (query) => {
     const cloneQuery = { ...query };

    // add square brackkets to identify array with single value
    Object.keys(cloneQuery).forEach((item) => {
      if (
        Array.isArray(cloneQuery[item]) &&
        cloneQuery[item].length === 1 &&
        !item.includes('[]')
      ) {
        cloneQuery[`${item}[]`] = cloneQuery[item];
        delete cloneQuery[item];
      }
    });

    return qs.stringify(cloneQuery, {
      encode: false,
      indices: false,
      arrayFormat: 'comma',

  });
};

let parseQuery = (query) =>
    qs.parse(query, {
      comma: true,
    });


const a = {des: ['hanoi']};

const b = {des: ['hanoi','hai phong']};


console.log(stringifyQuery(a)); // des[]=hanoi
console.log(stringifyQuery(b)); // des=hanoi,hai phong
console.log(parseQuery(stringifyQuery(a))); // { des: [ 'hanoi' ] }
console.log(parseQuery(stringifyQuery(b))); // { des: [ 'hanoi', 'hai phong' ] }

Is there any way shorter?

Thank

@Mohamadhassan98
Copy link

Mohamadhassan98 commented Aug 11, 2021

Two years of opening this issue and yet no solution?
Yet this problem exists with "repeat" format as well.

qs.stringify({a: 12, b: [1, 2], c: [1]}, { arrayFormat: 'repeat'}); // => a=12&b=1&b=2&c=1
qs.parse("a=12&b=1&b=2&c=1", { arrayFormat: 'repeat'}); // => {a: 12, b: [1, 2], c: 1}

No solution so far?

@DV8FromTheWorld

This comment has been minimized.

@ljharb
Copy link
Owner

ljharb commented Aug 14, 2021

@Mohamadhassan98 "no solution" because nobody's submitted a PR that fixes it.

Anyone who wants to send a PR would be most appreciated.

@ljharb
Copy link
Owner

ljharb commented Sep 16, 2023

Howdy, folks that care about the comma array format!

My intuition is that what you want most is either to be able to round-trip things between parse and stringify (which requires [] brackets to be added on one-item arrays in stringify, or on all of them); or, to match the API of a specific server implementation.

If the latter, can folks please describe exactly what they're using and what format it requires? If it supports what qs calls brackets, indices, or repeat, please also explain why you need to use comma over those.

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

Successfully merging a pull request may close this issue.

6 participants