Feature> emptyValue options for parse & stringify #226

timhwang21 · 2017-09-16T19:00:38Z

Addresses #223.

Behavior:

When parsing and encountering empty values, replaces them with
opt.emptyValue (default to '')
When stringifying and encountering value === opt.emptyValue, outputs a key
without a value

timhwang21

Parsing works properly, but I have a question regarding the best way to handle a certain stringification behavior. Thanks!

timhwang21 · 2017-09-16T19:03:11Z

lib/stringify.js

@@ -27,13 +29,15 @@ var defaults = {
    },
    skipNulls: false,
    strictNullHandling: false
+    // emptyValue: some flag


I'm not sure what the best way to handle this case is, as there needs to way to recognize "user provided no option" so the default behavior of outputting foo= as opposed to foo can be used.

I've seen things like using the global Infinity, storing the option in an object and using hasOwnProperty, etc. among others but not sure what you'd prefer. Thanks!

Hmm, I don't think I'm understanding the question here. Can you restate? A test case (that currently fails) would be most helpful.

Yeah -- let's say we don't provide a default. This means when stringifying { foo: undefined } we get 'foo' whereas before we get '' (line 77). If we do provide a default, stringifying { foo: [whatever our chosen default is]} gives 'foo' instead of whatever it gave before my change. So without some more involved code, stringifying whatever value we choose as the default would change from whatever it was before to 'foo'.

I'd like this not to be a breaking change, so qs.stringify({ foo: undefined }) needs to return the same value in master as it does in this PR.

cc @timhwang21, have you had any time to work on this PR?

In all honesty, no, this entirely fell off my radar. Will catch up on the past few years of discussion 😅

lib/parse.js

lib/stringify.js

ljharb · 2018-11-23T19:15:29Z

@timhwang21 is this still something you're interested in?

timhwang21 · 2018-11-23T20:00:51Z

@ljharb sure! Sorry this fell off my radar as we ended up solving the problem we were facing on our end.

timhwang21 · 2018-11-24T22:33:19Z

lib/stringify.js

-    if (obj === null) {
-        if (strictNullHandling) {
+    if (obj === null || obj === emptyValue) {
+        if (strictNullHandling || (obj === emptyValue && emptyValue !== null)) {
            return encoder && !encodeValuesOnly ? encoder(prefix, defaults.encoder, charset) : prefix;


This code is sort of confusing, but I think this results in the most expected outcomes:

No options: { a: null } becomes 'a='

strictNullHandling: { a: null } becomes 'a'

emptyValue: 'something': { a: 'something' } becomes 'a'

emptyValue: null: { a: null } becomes 'a=' (corner case; for null we defer to strictNullHandling)

emptyValue: null + strictNullHandling: { a: null } becomes 'a' (same as case 1; emptyValue: null is redundant here)

The relevant cases for this patch are 3, 4 and 5. Without the additional check on line 71, case 3 would give 'a='. However to me it seems if the user provides emptyValue they implicitly want "strict empty handling" to be true, as I don't think returning 'a=' for 3 is ever a useful outcome.

To avoid changing existing behavior, when emptyValue is null we require strictNullHandling on top of that.

An alternative would be to make emptyValue ALWAYS assume strictNullHandling is true, even for null. We'd have to change the default value from null to some flag that would never be used as an actual value, like Infinity or something.

Hmm, I'm not sure I agree - a= explicitly means "empty" to me, whereas a could be shorthand for a: true. I think I might expect case 3 to become a=, and case 6 (emptyValue: 'something' + strictNullHandling) makes { a: 'something' } become a?

I do agree that case 3 giving a= is more expected, but I think it's less useful. I'm imagining the following:

User does qs.parse('foo&bar=1', { emptyValue: true }) and gets { foo: true, bar: 1 }

User increments bar by 1 and re-serializes: qs.stringify({ foo: true, bar: 2 }, { emptyValue: true }). I'd expect to get foo&bar=2 which follows the format of the original input, not foo=&bar=2 here.

Regardless... I do agree that requiring strictNullHandling: true for this behavior for stringify does feel more logical. Returning a just strikes me as more in line with the use case this PR is meant to address.

I guess the confusion is because we have an implicit empty value of null, and a separate “strict null handling” var. if it was “strict empty value handling”, then it’d be easy to make it make sense. I’m not sure what the best approach here is.

Hm. What if we move the check for emptyValue to a block under the check for null? Because line 75 sets obj = '' if !strictNullHandling, obj will never equal emptyValue's implicit default value of null. Then the concerns will be more segregated.

I think it makes sense to forbid the strictNullHandling option entirely when emptyValue is not null.

However, then it may make sense to add a new option, strictEmptyHandling, that subsumes strictNullHandling in all cases?

+1 on the first part, that makes it more consistent with parse behavior.

I'm honestly still unconvinced on the second part, as I still don't think anyone would ever want to use stringify + emptyValue without also wanting "strict" behavior. It just seems like it would lead to user error since the desired outcome requires emptyValue to always be used in tandem with strictFooHandling...

Both with and without the strict option, what’s the roundtrip behavior of a null?

stringify({ a: null }) // => 'a=' parse('a=') // => { a: '' } // strict - matches stringify({ a: null }, { strictNullHandling: true }) // => 'a' parse('a', { strictNullHandling: true }) // => { a: null }

I'd expect emptyValue: 'foobar' to basically be strictFoobarHandling: true:

stringify({ a: 'foobar' }, { emptyValue: 'foobar' }) // => 'a' parse('a', { emptyValue: 'foobar' }) // => { a: 'foobar' }

lib/parse.js

ljharb · 2018-11-25T05:09:46Z

lib/stringify.js

-    if (obj === null) {
-        if (strictNullHandling) {
+    if (obj === null || obj === emptyValue) {
+        if (strictNullHandling || (obj === emptyValue && emptyValue !== null)) {
            return encoder && !encodeValuesOnly ? encoder(prefix, defaults.encoder, charset) : prefix;


Hmm, I'm not sure I agree - a= explicitly means "empty" to me, whereas a could be shorthand for a: true. I think I might expect case 3 to become a=, and case 6 (emptyValue: 'something' + strictNullHandling) makes { a: 'something' } become a?

issuefiler · 2020-01-15T04:55:01Z

I suggest the following.

INPUT
OUTPUT

""
{}

"a"
{a: null}

"a="
{a: ""}

"a=null"
{a: "null"}

"a[0]" (arrayFormat: "indices")
{a: []}

"a[0]=" (arrayFormat: "indices")
{a: [""]}

"a[]" (arrayFormat: "brackets")
{a: []}

"a[]=" (arrayFormat: "brackets")
{a: [""]}

{a: undefined}
""

{a: null}
"a"

{a: ""}
"a="

{a: "null"}
"a=null"

I believe interpreting the presense of a key as its value being truthy is up to the users.
qs should, by default, just focus on conveying what keys are “defined” and what values, if any, they hold.
qs doesn’t know whether "a" is boolean or not.

If how others interpret the stringified is concerned, why don’t you add “Do not convey (do omit) the presense of nulled keys” and “Do not convey (do omit) the presense of empty arrays” options?

jayaddison · 2020-05-11T09:56:43Z

Just adding a note that I'm interested in this functionality too.

While I mostly agree with @issuefiler's desired behaviour (a is null, a= is an empty string, 'undefined' implies key-not-present, ...), I also agree with the suggestion that this should be an option via options.emptyValue.

Given that truthy/falsy checks can be tricky and non-obvious to reason about, it's worth being careful to reduce impact to existing codebases (especially since there are many of them) when they upgrade qs.

timhwang21 · 2020-07-11T17:03:21Z

@ljharb after thinking about this some more, I'm liking the idea of strictEmptyHandling (and skipEmpty as well) you suggested. I'm proposing the following API:

Existing options:

strictNullHandling -- Now no longer does anything by itself. When set to true, sets strictEmptyHandling to true and sets emptyValues to null.
skipNulls -- Now no longer does anything by itself. When set to true, sets skipEmpty to true and sets emptyValues to null.

New options:

emptyValues -- Defaults to null. Represents the value qs treats as "empty."
strictEmptyHandling
- parse -- when set to true and given a value without a trailing =, parses as emptyValue instead of ''.
- stringify -- when set to true and given a k-v pair with where value === emptyValue, returns a the key without an equals sign.
skipEmpty
- ~~parse~~ -- not implemented
- stringify -- when set to true and stringifying a k-v pair where value === emptyValue, returns the empty string.

Edge cases:

When stringifying, we cannot treat multiple values as empty. We can allow passing an array emptyValues to stringify; however, this breaks the parallel options setup between stringify and parse, because multiple empty values don't make sense for parse. The use case would be that when stringifying, we can do things like treat both null and '' as empty. Alternatively, we can stay with the API of the current PR and keep the concept of empty and strict null entirely separate; however, I feel this makes the API harder to understand.
strictNullHandling: true or skipNulls: true AND emptyValue != null. The conflict arises from strictNullHandling wanting to set emptyValue to null. We can either have this throw an error like my current PR does or have the user-specified emptyValue override null. I think the latter is reasonable but confusing.

Summary:

If not passing any value to emptyValue (meaning the default of null is used), strict{Null,Empty}Handling and skip{Nulls,Empty} are equivalent.
Existing code that doesn't specify the new options will see no change in behavior.
I think we should choose one of the two options I proposed above for handling the need to serialize BOTH null an '' as an empty value. This seems like a very reasonable and desirable use case. I personally favor options.emptyValue: any for parse and options.emptyValues: any[] for stringify.

Please let me know what you think. Thanks!

ljharb · 2021-01-13T16:03:54Z

@timhwang21 I like your new API suggestion. I like throwing an error when conceptually conflicting options are passed.

I'm not sure I understand the last bullet point in the "Summary", but if you're able to update the PR, maybe it will become clear to me :-)

ljharb · 2024-06-11T12:01:49Z

@timhwang21 do you no longer have this use case? I'd still love to see this PR updated to match #226 (comment) so I can land it :-)

timhwang21 · 2024-06-11T16:59:35Z

I do not, and I unfortunately am not going to have time to work on this. I'm cleaning up my stale PRs because my dashboard is getting hard to manage.

ljharb · 2024-06-11T17:01:28Z

It's really unfortunate that github's PR dashboard incentivizes people to close old PRs :-/ if this stays open, then someone else can take up the work very easily.

timhwang21 commented Sep 16, 2017

View reviewed changes

ljharb added needs user followup semver-minor: new features parse stringify labels Nov 23, 2018

timhwang21 force-pushed the 223-emptyValue branch from 5713e30 to ea789cc Compare November 24, 2018 22:28

timhwang21 changed the title ~~WIP: emptyValue options for parse & stringify~~ Feature> emptyValue options for parse & stringify Nov 24, 2018

timhwang21 commented Nov 24, 2018

View reviewed changes

ljharb reviewed Nov 25, 2018

View reviewed changes

timhwang21 force-pushed the 223-emptyValue branch from ea789cc to d159599 Compare November 25, 2018 16:18

ljharb mentioned this pull request Jan 1, 2019

How to flag skip empty sring #292

Open

ljharb mentioned this pull request May 10, 2019

arrayformat as comma with empty array or objects #310

Closed

ljharb mentioned this pull request Jul 8, 2020

Feature > Option to skip empty string at "qs.stringify()" #372

Closed

WIP: rebase and update to match current code style

ee6ec28

experimental: subsume strictNullHandling in emptyValue

66f1bf6

timhwang21 force-pushed the 223-emptyValue branch from d159599 to 66f1bf6 Compare July 11, 2020 17:04

ljharb marked this pull request as draft January 13, 2021 16:04

ljharb force-pushed the master branch from e3b562a to facbd0e Compare February 17, 2021 00:51

ljharb force-pushed the master branch from 44b6031 to 24c19cc Compare November 9, 2021 19:52

ljharb force-pushed the main branch from a5371f6 to c8a269f Compare February 29, 2024 23:18

timhwang21 closed this Jun 11, 2024

ljharb reopened this Jun 11, 2024

timhwang21 closed this Jun 11, 2024

timhwang21 deleted the 223-emptyValue branch June 11, 2024 17:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature> emptyValue options for parse & stringify #226

Feature> emptyValue options for parse & stringify #226

timhwang21 commented Sep 16, 2017

timhwang21 left a comment

timhwang21 Sep 16, 2017

ljharb Sep 19, 2017

timhwang21 Sep 19, 2017

ljharb Sep 20, 2017

ljharb Jul 8, 2020

timhwang21 Jul 8, 2020

ljharb commented Nov 23, 2018

timhwang21 commented Nov 23, 2018

timhwang21 Nov 24, 2018 •

edited

Loading

ljharb Nov 25, 2018 •

edited

Loading

timhwang21 Nov 25, 2018

ljharb Nov 25, 2018

timhwang21 Nov 25, 2018

ljharb Nov 25, 2018

timhwang21 Nov 25, 2018

ljharb Nov 25, 2018

timhwang21 Nov 25, 2018

ljharb Nov 25, 2018 •

edited

Loading

issuefiler commented Jan 15, 2020

jayaddison commented May 11, 2020

timhwang21 commented Jul 11, 2020

ljharb commented Jan 13, 2021

ljharb commented Jun 11, 2024

timhwang21 commented Jun 11, 2024

ljharb commented Jun 11, 2024

Feature> emptyValue options for parse & stringify #226

Feature> emptyValue options for parse & stringify #226

Conversation

timhwang21 commented Sep 16, 2017

timhwang21 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb commented Nov 23, 2018

timhwang21 commented Nov 23, 2018

timhwang21 Nov 24, 2018 • edited Loading

Choose a reason for hiding this comment

ljharb Nov 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb Nov 25, 2018 • edited Loading

Choose a reason for hiding this comment

issuefiler commented Jan 15, 2020

jayaddison commented May 11, 2020

timhwang21 commented Jul 11, 2020

ljharb commented Jan 13, 2021

ljharb commented Jun 11, 2024

timhwang21 commented Jun 11, 2024

ljharb commented Jun 11, 2024

timhwang21 Nov 24, 2018 •

edited

Loading

ljharb Nov 25, 2018 •

edited

Loading

ljharb Nov 25, 2018 •

edited

Loading