tc39 / proposal-upsert

ECMAScript Proposal, specs, and reference implementation for Map.prototype.upsert
https://tc39.es/proposal-upsert/
MIT License
204 stars 14 forks source link

Iterating on the design #20

Closed codehag closed 1 month ago

codehag commented 4 years ago

We reviewed this proposal back when it was presented, but I didn't manage to get our notes up.

One of the major comments was that the api is unintuitive as it tries to do too many things. There might be a better way to achieve the desired effect. A number of folks mentioned defaultdict as being the thing that they miss when writing js. The most common usage is the following: if (!map.has(k)) { m.set(k, []) }; m.get(k).push(val);

A suggestion from @jorendorff was new Map ( [iterable [, valueFn]] ). When the user calls map.get(key) with no second argument, and no entry for key exists, and valueFn was specified, it’ll call valueFn(key) and insert and return the resulting value. Same for WeakMap.

ljharb commented 4 years ago

That sounds workable to me (altho i'd want the valueFn to be in an options bag, so more "internal hooks" could be added in the future).

hax commented 4 years ago

Maybe we could simply overload map.get(k) with map.get(k, valueFn)?

jorendorff commented 4 years ago

An options bag sounds good.

bmeck commented 4 years ago

I think the function being internal state to the collection might cause some usability issues as people have existing code that does the following:

let value = map.get(value) || generateDefault();
// mutate /  replace value
map.set(key, value);

Having .get do a defaulting operation here would mean it would suddenly not take that branch (could be seen as intentional arguably but I am not convinced at a glance that is good) and that multiple places using different values would be unable to give different defaults. You can imagine scenarios like the following as well:

function init(k) {
  return map.get(k); // should synchronously introduce it if it doesn't exist
}

function result(k) {
  return map.get(k); // should create an error/tdz
}

Other possible things are stuff like inserting synchronous values if the value would be known at the time of the .get vs asynchronous values if the value is not available and might need to be fetched somehow.

Additionally, one of the big usages of mine is to update existing values. With Record/Tuple/primitives/frozen values/etc. this does lean towards needing an update function. I think having a defaulted value is useful, but I do not think it would be the best path at this time unless we can convince ourselves that these state-less/read-only values are not needing to be updated in the common case. In my experience, if you get a default value you often want to perform some kind of update operation on it afterwards anyway.

bmeck commented 4 years ago

I've moved feedback from several issues to a revised design in https://github.com/tc39/proposal-upsert/issues/21

bergus commented 3 years ago

The most common usage is the following: if (!map.has(k)) { m.set(k, []) }; m.get(k).push(val);

Don't forget about counters (or any other updates with immutable values, where you can't just push to the array):

map.set(k, 1 + (map.has(k) ? map.get(k) : 0))
map.set(k, map.has(k) ? map.get(k) + 1 : 1))

This use case is not covered by default values. map.set(k, map.get(k, 0)+1) is closer but still doesn't get us all the way to a single (hash/tree) lookup.

dminor commented 1 month ago

The new design direction is focusing on the get/insert if missing usecase, with update being left for a future proposal.