qs icon indicating copy to clipboard operation
qs copied to clipboard

Feature> emptyValue options for parse & stringify

Open timhwang21 opened this issue 7 years ago • 6 comments

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 avatar Sep 16 '17 19:09 timhwang21

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

ljharb avatar Nov 23 '18 19:11 ljharb

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

timhwang21 avatar Nov 23 '18 20:11 timhwang21

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?

issuefiler avatar Jan 15 '20 04:01 issuefiler

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.

jayaddison avatar May 11 '20 09:05 jayaddison

@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!

timhwang21 avatar Jul 11 '20 17:07 timhwang21

@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 avatar Jan 13 '21 16:01 ljharb