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

URLSearchParams append/set: variadic values argument? #762

Open
bathos opened this issue Mar 14, 2023 · 6 comments
Open

URLSearchParams append/set: variadic values argument? #762

bathos opened this issue Mar 14, 2023 · 6 comments
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest topic: api

Comments

@bathos
Copy link

bathos commented Mar 14, 2023

When assembling URLs with parameters where the same key is used with multiple values, I’ve often needed to set those multiple values at once and found the URLSearchParams API surface slightly clunky for this. I’d mentioned this in another issue and @annevk pointed out that this could be facilitated backwards-compatibly without any new API surface just by making the set and append operations variadic. Instead of:

interface URLSearchParams {
  /* ... */
  undefined append(USVString name, USVString value);
  undefined set(USVString name, USVString value);
};

They would be:

interface URLSearchParams {
  /* ... */
  undefined append(USVString name, USVString ...values);
  undefined set(USVString name, USVString ...values);
};

An example of what this change facilitates would be the filtering or mapping of parameters as may occur when applying application-specific URL canonicalization rules or setting parameters based on the current state of a search form. Instead of:

let foos = usp.getAll("foo").filter(isFooValue);

usp.delete("foo"):

for (let foo of foos) usp.append("foo", foo);

One could write:

usp.set("foo", ...usp.getAll("foo").filter(isFooValue));

Is there interest from others in this change? It’s a minor ergonomics improvement, but I’d guess it’s also pretty low-hanging from an implementer POV.

@annevk annevk added topic: api needs implementer interest Moving the issue forward requires implementers to express interest addition/proposal New features or enhancements labels Mar 14, 2023
@jasnell
Copy link
Collaborator

jasnell commented Mar 14, 2023

I'm generally +1 on the idea but suggest that it should be separate function.... But can live with this approach.

@lucacasonato
Copy link
Member

It may be a bit of a hazard that a non variadic function suddenly becomes variadic. Is this something we have done previously?

@annevk
Copy link
Member

annevk commented Mar 18, 2023

I'm not sure, variadic made sense to me because of Array.prototype.push().

@bathos
Copy link
Author

bathos commented Mar 18, 2023

Operations have had new parameters added previously many times, right? Is there something that makes the variadic case more hazardous? If the hazard is that someone might have passed an argument by mistake which will no longer be ignored, it seems to apply equally to both.

@jasnell
Copy link
Collaborator

jasnell commented Mar 20, 2023

As I said, I can live with the variadic approach. I don't prefer it because discoverability is weakened. If I'm on one runtime that has implemented this and I write params.append("foo", "abc", "xyz") then run that code on another runtime that hasn't implemented this, it is not going to be immediately apparent why the "xyz" is not showing up, leading to subtle bugs. Alternatively, using a new method like params.appendAll(...), I would get a clear error when the API has not been implemented.

@bathos
Copy link
Author

bathos commented Mar 20, 2023

it is not going to be immediately apparent why the "xyz" is not showing up

That’s a good point. I have no strong preference myself for expanded signature vs new method but this appeared pretty clean to me and seemed to avoid confusion with other possible notions of “appending all”: this issue was spawned from another regarding a proposed appendAll which would take a record argument and seemingly would permit expressing only one value per key (come to think of it, maybe its author meant setAll instead of appendAll...?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest topic: api
Development

No branches or pull requests

4 participants