url icon indicating copy to clipboard operation
url copied to clipboard

URLSearchParams delete all vs delete one

Open cailyncodes opened this issue 7 years ago • 13 comments

The specification for URLSearchParams currently reads that delete should delete all pairs that have the supplied key. I think the ability to delete a specific key value pair should be added.

For example:

let url = new URLSearchParams("key1=value1&key1=value1a&key2=value2");
url.delete("key1","value1");
url.toString(); // key1=value1a&key2=value2

A use case for this is faceted searching. Say the user has selected three keys to facet with. Example url: http://example.com/?q=searchTerms&facet.category=selection1&facet.category=selection2&facet.category=selection3. Now the user wants to clear only selection2 from their search.

While it wouldn't be too difficult to use the current methods to obtain the desired output above (getAll the given key, delete all pairs with the given key, iterate over the getAll result adding back everything but the one pair), I think it goes against the goal of the URLSearchParams class as I understand it. It seems URLSearchParams tries to abstract away the string filtering and concatenations that were required for query params before. The workaround falls back into that extra manipulation approach.

I think that any of the following changes would solve this.

  1. Allow a second optional argument to be passed to delete that would specify the value of which key-value to delete if there were multiple key-value pairs with the given key.

Example: See above.

  1. Change the current definition of delete to require two arguments, a key and a value. The method would then delete the supplied key-value pair, if it exists. Introduce a new method, deleteAll, that behaves how delete does currently. This would follow the pattern of having both get and getAll methods.
let url = new URLSearchParams("key1=value1&key1=value1a&key2=value2&key2=value2b");
url.delete("key1","value1");
url.toString(); // key1=value1a&key2=value2&key2=value2b
url.deleteAll("key2");
url.toString(); // key1=value1a

For both options, in cases where there are repeats of the same key-value pair, all instances would be deleted. Example:

let url = new URLSearchParams("key1=value1&key1=value1&key2=value2");
url.delete("key1","value1");
url.toString(); // key2=value2

While I am more of a fan of option 2 because it lines up closer to the get/getAll methods, it requires a backwards incompatible change. Therefore, it would probably be best to go the route of option 1. (Arguably, a third option could be to keep 1 argument delete the same, introduce deleteAll and two argument delete, but is probably extra and unnecessary.)

What are other people's thoughts?

cailyncodes avatar Jul 12 '17 18:07 cailyncodes

I think I'd go with a second optional parameter (option 1). The get()/getAll() case is quite different conceptually as that's really about the predictability of the return value type, which we don't have to concern ourselves with here.

annevk avatar Jul 13 '17 08:07 annevk

Very fair. While I would personally still lean delete()/deleteAll() because I like the explicitness of it, my preference is only nominal, and I think either option is valid. So whichever is more receptive to implementation I would support.

cailyncodes avatar Jul 13 '17 13:07 cailyncodes

Just making sure, if there are multiple pairs with same name and value, all of such pairs will be deleted by the two-parameter variant, right?

TimothyGu avatar Jul 13 '17 13:07 TimothyGu

Correct. See the last example in the first post.

Additionally, I can't think of a case where only a subset would need to be deleted, so I don't think we need to consider a way to do that. If someone is considering the multiplicity of the pair, I would recommend instead passing an additional key-value pair with the count.

cailyncodes avatar Jul 13 '17 13:07 cailyncodes

Sorry, I missed that in my first reading. The idea SGTM. If people really want to do more advanced manipulations, there's always new URLSearchParams(manip(Array.from(params))).

TimothyGu avatar Jul 13 '17 13:07 TimothyGu

Any update?

ifdouglas-zz avatar Mar 11 '20 19:03 ifdouglas-zz

What would help in driving this forward is some indication that this is a common need (e.g., StackOverflow questions) and is being addressed in libraries (with the proposed semantics). That might make this a more compelling case to implementers.

annevk avatar May 04 '20 13:05 annevk

Just ran into this myself. I guessed that params.delete(key, valueMatch) would work but didn't, then came searching.

Use case is a typical sidebar product search filters. If you've got links to multiple brands of shoes maybe you'd like to click the brand to append it to the search params, and if it's already there, remove it.

Of course you can make an array, manipulate it, and construct a new one, but append sure is nicer than that. Deleting by key/value would be a nice bookend to append.

ryanflorence avatar Sep 15 '21 05:09 ryanflorence

I still don't think anybody has answered Anne's question, and unfortunately I didn't see any libraries or polyfills where this is implemented.

Re: the API design discussion upthread, I thought this analogous data structure could be a good place to start, since AFAICT it's basically the same thing. To save you a click, their answer is, delete("key1", "value1") removes an unspecified key1=value1 pair. (IMHO, if your backend service relies on treating a=1&b=2 differently from b=2&a=1, it's broken and you should fix it.)

thw0rted avatar Sep 15 '21 08:09 thw0rted

This issue now has 30 upvotes and 1 downvote, which would seem to be an "indication of common need". I think it's worth doing some work to investigate how feasible "option 1" is (provide an optional second parameter). If there's a lot of existing code that's passing a second parameter to delete() even though it doesn't do anything, then we might not be able to do option 1.

TimothyGu avatar Jan 05 '22 01:01 TimothyGu

I'm not sure the correct way to give an "indication of common need" but I would like to add my voice to those expecting delete(key, value) to be an option.

To give a concrete use case:

A query string containing filters that can have multiple. Such as ?author=Alice&author=Bob. Originally these were used as a single value ?authors=alice-bob but for various reasons (including querystring on NodeJS being deprecated) I looked into URLSearchParams to replace it and saw the ability to use getAll which seems an obvious case for this scenario.

But when wanting to delete a filter, it seems over the top to need to manipulate the array to rebuild the query just to delete one of the options.


For anyone finding this like I did via Google, here's how I handled the lack of this feature:

const query = new URLSearchParams("?author=Alice&author=Bob");
const allValues = query.getAll(key)
allValues.splice(allValues.indexOf("Alice"), 1);
query.delete(key);
allValues.forEach((val) => query.append(key, val));

console.log(query.toString()); // Query string will now just have Bob

ShaneHudson avatar May 31 '22 13:05 ShaneHudson

Now 50 upvotes. This would be realy handy feature :)

sis6326r avatar Jul 04 '22 00:07 sis6326r

When using @ShaneHudson's solution, be aware that allValues.indexOf("Alice") will return -1 if no key/value pair exists for that value. If query did not contain any entries with the value "Alice", then allValues.splice(-1, 1) would delete the last item in allValues, which is probably not what you want.

Here's a version which avoids this pitfall:

function deleteSearchParamByKeyValue (searchParams, key, value) {
  const allKeys = []
  const entriesToKeep = []

  for (const [k, v] of searchParams.entries()) {
    if (k === undefined || v === undefined) continue
    allKeys.push(k)
    if (k === key && v === value) continue
    entriesToKeep.push([k, v])
  }

  for (const k of allKeys) {
    searchParams.delete(k)
  }

  for (const [k, v] of entriesToKeep) {
    searchParams.append(k, v)
  }
}

Another difference is that this version will delete all entries which match the given key and value, whereas @ShaneHudson's version will delete only the first match.

josephmturner avatar Aug 21 '22 19:08 josephmturner

Today I discovered you can have multiple params with the same key. Today it popped up in my twitter time line: post from Ryan Florence and reply from Jake Archibald linking to an article. And today I needed the feature to implement a react custom hook to synchronize component state with the URL search params part. What a day.

Yes, we need this feature.

teetotum avatar Nov 22 '22 16:11 teetotum

If we do this we should also change FormData in the same way.

Since my last comment here I'm glad to see @thw0rted's comment about precedence in other code. I think ideally we have some more of that to ensure we're doing the right thing.

@valenting @domenic @tabatkins thoughts on adding this?

annevk avatar Nov 23 '22 14:11 annevk

Just to look for some more precedent:

  • C++'s std::multimap has an erase() method which can either wipe all the values for a given key, or remove a single k/v pair at a particular position, so it counts as precedent
  • We already saw the Java precedent, which offers "erase all" and "erase one" under different names
  • Scala has the ability to remove a key/value pair, but this impl, at least, can't have repeated pairs since the values are stored in a Set.
  • VertX, a java library only has "erase all". (It uses MultiMap specifically for handling http headers, which is interestingly close to our use-case.)
  • Apache's MultiValuedMap has both. (It's not clear from the docs if its "remove one" will kill all matching k/v or just a single one.)

This is just me looking at the first two pages of Google results, so I think it's fairly well established that "remove one k/v pair" is a reasonable part of a multimap API.

Re: whether it removes a single matching k/v pair or all matching pairs, I can see arguments either way, but I lean towards just removing one. (Specifically the first or last.) Most of the time it won't matter because you won't have dupes anyway, but sometimes you have dupes quite purposely and we can't reasonably guess whether someone wants to remove all of them or just one. If we went with "remove all" then the people wanting "remove one" behavior would have to do essentially the same workarounds as shown in this thread, while if we went with "remove one" then people wanting to remove all would have a much easier "remove in a while loop" workaround.

(Ugh, the lack of a 2-arg has() method means they'd have to do while(params.getAll(key).contains(value)) params.delete(key, value);, but that's something we can solve later, and it's still easier than pulling the values out, removing one, and putting them back.)

tabatkins avatar Nov 23 '22 16:11 tabatkins

Would there be no scope to pass an argument for removing all matching pairs with the default being singular (or vice versa)? I definitely think tbh the most common situation is that if someone has tried to remove a pair, they will expect the result to not have the pair. On 23 Nov 2022, 4:53 PM +0000, whatwg/url @.***>, wrote:

whether it removes a single matching k/v pair or all matching pairs, I can see arguments either way, but I lean towards just removing one

ShaneHudson avatar Nov 23 '22 18:11 ShaneHudson

I don't think there's a clear "most common" situation. For example, if you're using PHP it's common to have several inputs named foo[], which will translate into $_GET['foo'] being an array of the values on the server-side, and it's not unreasonable for several of these to have the same value, too. I can easily imagine wanting to remove all the duplicate values or just a single one, depending on your specific use-case.

tabatkins avatar Nov 23 '22 19:11 tabatkins

I think every use case presented could be addressed with clean ergonomics, without changing the meaning of any current methods, by adding:

  • has(key, value): true if key=value is in the collection
  • delete(key, value): deletes one random pair (as with Guava MultiMap). Could maybe return count of remaining key=value pairs, or just has(key,value), instead of undefined?
  • set(key, value1, value2, value3, ...): current set overwrites all values for the given key. Give it a rest parameter (like Array#push) and then I can write url.set(key, ...url.getAll(key).filter(v => v !== value)), which is effectively deleteAll(key, value)

thw0rted avatar Nov 23 '22 21:11 thw0rted

I created a polyfill to add support for .delete(key,value) I think I will also add support for .has(key,value)

teetotum avatar Nov 23 '22 23:11 teetotum

I like the idea of overloading has() at the same time. (set() would be more complicated given FormData. Separate issue if you want to pursue that please.)

It sounds like there's three different use cases for deleting by key and value:

  • Remove all. This would be while(sp.has(key, value)) sp.delete(key, value) under delete one semantics. Not great in my opinion.
  • Remove duplicates. I think this is already covered through sp.set(key, value). Potentially with a sp.has(key, value) conditional, depending on what local knowledge you have.
  • Remove one. This is the use case that is a bit unclear to me. Giving it the most straightforward API seems rather suspect.

My inclination based on this analysis would be to go with removal all semantics for delete(key, value) and maybe offer delete(key, value, { onlyFirst:true }) if someone comes up with a great use case.

(HTTP headers are represented by Headers and while that used to be more similar (it had getAll()), it was simplified to more closely match the actual HTTP semantics and as such it's no longer a good match for a multimap.)

annevk avatar Nov 24 '22 10:11 annevk

I'm trying to consider this in light of what an eventual JS multimap might look like too, so I'm assuming all use-cases are valid a priori right now.

My issue here is that the existing methods that duplicate Map methods all treat it like a normal Map, with a single value per key, while the methods that recognize this as a multimap all treat it like an ordered list of arbitrary values per key. If calling params.append(k, v); params.append(k,v); results in the key having two values, I don't think it's consistent to have params.delete(k, v) delete both of them with a single call. You'd be treating the values like they're a Set instead of an Array, but only for that one method.

tabatkins avatar Nov 29 '22 22:11 tabatkins

We do that for the keys as well so I don't see why we wouldn't do the same for the values. If you really want delete-a-single-entry semantics it probably makes more sense to pass an index.

annevk avatar Nov 30 '22 09:11 annevk

Apologies, I'm not sure what you mean by "we do that for the keys".

tabatkins avatar Nov 30 '22 17:11 tabatkins

You can have multiple entries with the same key and all will be deleted.

annevk avatar Dec 01 '22 07:12 annevk

I agree. I would aim for consistency with the existing behavior of URLSearchParams functions.

URLSearchParams.delete(key) deletes all entries with matching key

const params = new URLSearchParams('foo=1&foo=2&bar=3&bar=3');
params.delete('foo');
params.toString(); // 'bar=3&bar=3'

URLSearchParams.delete(key, value) deletes all entries with matching key and value

const params = new URLSearchParams('foo=1&foo=2&bar=3&bar=3');
params.delete('bar', '3');
params.toString(); // 'foo=1&foo=2'

teetotum avatar Dec 01 '22 10:12 teetotum

I agree, and you still have the ability to manipulate the array directly like we do currently in the case of wanting a specific one deleted. On 1 Dec 2022 at 10:17 AM +0000, Martin @.***>, wrote:

I agree. I would aim for consistency with the existing behavior of URLSearchParams functions. URLSearchParams.delete(key) deletes all entries with matching key const params = new URLSearchParams('foo=1&foo=2&bar=3&bar=3'); params.delete('foo'); params.toString(); // 'bar=3&bar=3' URLSearchParams.delete(key, value) deletes all entries with matching key and value const params = new URLSearchParams('foo=1&foo=2&bar=3&bar=3'); params.delete('bar', '3'); params.toString(); // 'foo=1&foo=2' — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

ShaneHudson avatar Dec 01 '22 10:12 ShaneHudson

You can have multiple entries with the same key and all will be deleted.

Right, because you're calling .delete(key), which it inherits from masquerading as a Map; under the model that those methods act under, each key is unique among the k/v pairs, so deleting the key needs to ensure that afterwards the key is not present in the map. Essentially it just pretends that all subsequent k/v pairs with a repeated key don't exist, and ensures that the results after calling the methods accord with that - returning only the first from .get(), replacing all of them with .set(), deleting all of them with .delete().

But for the methods that aren't borrowed from Map (append, getAll), they use the mental model of a list of arbitrary k/v pairs, where none of the keys, values, or k/v pairs are necessarily unique. It would be odd if the 2-arg delete method operated under a third model, where the keys and values aren't unique but the k/v pairs are.

tabatkins avatar Dec 01 '22 23:12 tabatkins

I'm not sure it's that weird, as unlike append() or getAll(), position/index isn't part of the API. And I think more importantly, we don't have a good use case for deleting the first instance (presumably it would match set()/get() here).

annevk avatar Dec 02 '22 07:12 annevk

Just commenting that I'm hand rolling an implementation and that several of the proposed solutions above are satisfactory improvements for workflows that involve toggling key/value pairs.

trawn3333 avatar Mar 11 '23 18:03 trawn3333