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

Custom stringify arrays and objects #365

Draft
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

leninlin
Copy link

@leninlin leninlin commented May 2, 2020

I added the ability to compile the query string using a custom function for arrays and objects.

I need this to work with the Python REST framework (https://github.com/AltSchool/dynamic-rest):

qs.stringify({
  include: [ 'name', 'age' ],
  filter: {
    'age.gte': 18
  }
});
// include[]=name&include[]=age&filter{age.gte}=18

I added a function with curly brackets to the default list, but custom functions for the future will be useful for such situations.

ljharb and others added 30 commits August 16, 2019 19:43
A new fourth argument is now provided when invoking the decoder inside parse,
this argument represents whether the first argument (str) is a key or a value.
This can then be used to decode key and values different within the decode function.

A test has been added for this new behavior.
… treated as normal text

Co-authored-by: Mohamed Omar <mohamed.ahmed.c209@gmail.com>
Co-authored-by: Quentin de Longraye <quentin@dldl.fr>
Copy link
Owner

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any changes to stringify would need to be mirrored in parse, so things can round trip.

I'm not yet convinced that this complexity is worth adding, but I appreciate you've made it appropriately generic.

dist/qs.js Outdated Show resolved Hide resolved
@leninlin
Copy link
Author

leninlin commented May 2, 2020

Any changes to stringify would need to be mirrored in parse, so things can round trip.

I'll think about it now

I'm not yet convinced that this complexity is worth adding, but I appreciate you've made it appropriately generic.

No problems. I can use fork for myself.

@leninlin
Copy link
Author

leninlin commented May 3, 2020

Maybe something like that?

@leninlin leninlin requested a review from ljharb May 31, 2020 14:55
Copy link
Owner

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused why there's both an array replacer and an object replacer, since for parse, they both do the same thing.

Comment on lines +149 to +153
You may set `objectFormat: 'curly'` to enable curly brackets notation:

```javascript
var withDots = qs.parse('a{b}=c', { objectFormat: 'curly' });
assert.deepEqual(withDots, { a: { b: 'c' } });
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is an extremely confusing and strange notation i've never seen anywhere else; i'm fine adding a function form so someone can do this themselves, but i'd need a lot of convincing that this format deserves first-class support.

Comment on lines +424 to +425
qs.stringify({ a: { b: { c: 'd', e: 'f' } } }, { arrayFormat: 'curly' });
// 'a{b}{c}=d&a{b}{e}=f'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similarly here

Suggested change
qs.stringify({ a: { b: { c: 'd', e: 'f' } } }, { arrayFormat: 'curly' });
// 'a{b}{c}=d&a{b}{e}=f'

var key = givenKey;
if (typeof options.arrayFormat === 'function') {
key = options.arrayFormat(key);
} else if (options.arrayFormat in arrayKeyReplacer) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
} else if (options.arrayFormat in arrayKeyReplacer) {
} else if (has.call(arrayKeyReplacer, options.arrayFormat)) {

}
if (typeof options.objectFormat === 'function') {
key = options.objectFormat(key);
} else if (options.objectFormat in objectKeyReplacer) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
} else if (options.objectFormat in objectKeyReplacer) {
} else if (has.call(objectKeyReplacer, options.objectFormat)) {

Comment on lines +18 to +20
curly: function curly(key) {
return key.replace(/\{([^{}[]+)\}/g, '[$1]');
},
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
curly: function curly(key) {
return key.replace(/\{([^{}[]+)\}/g, '[$1]');
},

var generateObjectPrefix;
if (opts && typeof opts.objectFormat === 'function') {
generateObjectPrefix = opts.objectFormat;
} else if (opts && opts.objectFormat in objectPrefixGenerators) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
} else if (opts && opts.objectFormat in objectPrefixGenerators) {
} else if (opts && has.call(objectPrefixGenerators, opts.objectFormat)) {

Comment on lines +252 to +256
} else if (opts && opts.allowDots) {
generateObjectPrefix = objectPrefixGenerators.dots;
} else {
generateObjectPrefix = objectPrefixGenerators.brackets;
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems like it should be handled by the option normalization and the default?

st.deepEqual(qs.parse('a[]=b&a[]=c', { arrayFormat: 'comma' }), { a: ['b', 'c'] });
st.deepEqual(qs.parse('a[0]=b&a[1]=c', { arrayFormat: 'comma' }), { a: ['b', 'c'] });
st.deepEqual(qs.parse('a=b,c', { arrayFormat: 'comma' }), { a: 'b,c' });
st.deepEqual(qs.parse('a=b,c', { arrayFormat: 'comma' }), { a: ['b', 'c'] });
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this shouldn't change, or else it's a breaking change

@ljharb ljharb added the parse label Jun 20, 2020
@ljharb
Copy link
Owner

ljharb commented Jan 13, 2021

@leninlin are you still interested in completing this PR?

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

Successfully merging this pull request may close these issues.

None yet

3 participants