Make URLSearchParams' record constructor more flexible

[TimothyGu]

Now that #27 is merged, creation of URLSearchParams objects from an object/record has gotten a lot easier. However, one problem with the new syntax is that duplicated keys are not allowed with this new syntax.

I propose making the following change to the type of init parameter of the URLSearchParams constructor, so that more complex objects a la Node.js' querystring module:

-(sequence<sequence<USVString>> or record<USVString,  USVString                        > or USVString)
+(sequence<sequence<USVString>> or record<USVString, (USVString or sequence<USVString>)> or USVString)

This way, Node.js users will be able to easily interoperate between URLSearchParams objects with native querystring module.

---

Preview | Diff

[domenic]

I don't believe USVStrings and sequences are distinguishable, so this is invalid Web IDL, I believe.

In general, -1. The way you represent maps in ES is with the [[x, y], [x, z]] syntax. The { x: y } syntax is a convenient abstraction for the simplest case, but should not be mutated to cover cases it wasn't meant to.

[annevk]

They are per https://heycam.github.io/webidl/#dfn-distinguishable. But I agree that this seems wrong. It would also be inconsistent with what we did for Headers.

[annevk]

So to be clear, the duplicated keys scenario is handled by nested sequences ([['x', 'y'], ['x', 'z']]). But from your feedback it sounds like that is something that Node.js has not used thus far.

I still rather not fix this and await further developer feedback.

[jasnell]

Nested sequences are used in a couple of obscure ways internally but not really exposed to the general Node.js user. For instance, I'm using a nested sequence for transmitting headers to the underlying native binding in the http/2 implementation in core but the user facing API translates those into a dictionary object with a null prototype, primarily due to ease of use concerns.

For instance, when a multi-valued http/2 header is received, the structure handed off to the user looks like:

{
  "header1": "value",
  "header2": ["value1", "value2"]
}

As opposed to:

[
  ["header1": "value"],
  ["header2": "value1"],
  ["header2": "value2"]
]

The former is what most Node.js users would expect to be able to pass as input.

[Janpot]

As for "developer feedback": If I recall correctly, libraries like express and request often have headers in that object format. Maybe a bit more niche, but the chrome devtools protocol has headers in this format as well. I've been bitten by bugs in my applications when I didn't expect an array in those objects. One of those bugs led me to open #206 as I assumed URLSearchParams was compatible with this format.
I'd say both libraries are big enough to justify this feature request though.

[domenic]

None of this seems like justification for adding two ways to do the same thing, especially when this new proposal has the downside of mixing types (arrays sometimes, strings other times).

[Janpot]

I agree that the object format is not the best representation for headers, but if you're going to support it, then I think you should also support the array values. As I can imagine developers would blindly pass in node header/querystring objects into URLSearchParams without realising the formats are not compatible. I think it's just going to be a source of bugs then.
You seem quite opposed to the idea, I don't have that strong feelings towards it so I'd recommend to do away with the object format altogether. No reason to have two ways of doing things, especially if one of those is incompatible with existing solutions.

[domenic]

I guess my remaining question is whether people really even know that you can mix arrays and strings in the Node.js format. I didn't until this issue. I'd always thought that once you've outgrown the simple-case object string->string format, you had to move to the general-case nested-arrays format. But maybe there are some developers out there who figure out that you can instead move to an intermediate string->string+array format? I kind of doubt it, but I don't have any data, just a gut feeling.

[Janpot]

I don't think a lot of people know. And I always forget it myself.
I'd say that following use case is not that uncommon though:

const querystring = require('querystring');
querystring.parse('a=1&a=2');
// { a: [ '1', '2' ] }

also set-cookie headers on requests is a quite common use case.

To be clear, I don't really like that API too, but it's there now, and if you're going to support only half of it, I think that's worse than not supporting it at all.

[TimothyGu]

I guess my remaining question is whether people really even know that you can mix arrays and strings in the Node.js format.

I don't know about "Node.js developerd" in general, but the querystring documentation has a pretty clearly stated example showing this format.

And to be honest, personally I'd rather write the object-array syntax simply because it's less typing.

[annevk]

Note that one problem of using this format is that you lose ordering of keys with the same name. It doesn't seem entirely unreasonable to support though, but if we add it we should also add it to the Headers objects.

[TimothyGu]

but if we add it we should also add it to the Headers objects.

Agreed. But this change gotta start somewhere 😀

[jasnell]

Ordering of the keys is indeed an important consideration. I'm not going to argue that we need to have the object-array approach. I'm only going to say that it's likely what many Node.js developers will likely expect given the pattern established by other APIs... e.g.

const qs = require('querystring');
console.log(qs.stringify({a:[1,2]}));
  // prints: a=1&a=2

This is not an uncommon pattern in Node.js and I have absolutely no doubt that we will receive questions and issue reports about it not being supported.

[joyeecheung]

Possibly related: nodejs/node#3591 and nodejs/node#3591 (comment)

[TimothyGu]

Any updates on this?

[annevk]

I'm still a little uncertain.

The Node.js documentation gives this as the result of query string parsing. But if you were to use URLSearchParams instead to parse a query string, you would not get that kind of representation out of it. So I don't really see how this helps there.

It does help with serializing to a query string from an object using this representation.

@cdumez @bakulf @mikewest @travisleithead thoughts?

[Janpot]

Another option could be to only allow arrays as values?

{
  "header1": ["value"],
  "header2": ["value1", "value2"]
}

[TimothyGu]

@annevk, the Node.js documentation has since been updated with the record constructor added with a big warning, one that I'd like to remove:

Note: Unlike querystring module, duplicate keys in the form of array values are not allowed. Arrays are stringified using array.toString(), which simply joins all array elements with commas.

const { URLSearchParams } = require('url');
const params = new URLSearchParams({
  user: 'abc',
  query: ['first', 'second']
});
console.log(params.getAll('query'));
  // Prints ['first,second']
console.log(params.toString());
  // Prints 'user=abc&query=first%2Csecond'

@Janpot, why? It wouldn't solve the compatibility problem with Node.js, is harder to write, and would break compatibility with user agents that already implemented the record constructor.

[Janpot]

@TimothyGu ach, you're right, something short circuited in my brain. Ignore my comment.