tc39 / proposal-upsert

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

(re?)consider no-op for missing key, and missing insertion handler #46

Closed ghost closed 1 month ago

ghost commented 2 years ago

This example snippet appears in the README, for just update if present:

if (map.has(key)) {
  map.emplace(key, {
    update: (old) => old.doThing()
  });
}

But it could be fairly useful to have a "just update if present", opposite to the just insert if missing semantics, in that the checking for whether the key is present, is also coalesced into the operation, and handled by the engine instead.

The FAQ mentions gives the rationale for this behaviour under why error if the key doesnt exist and no insert handler is provided:

This keeps the return type constrained to the union of insert and update without adding undefined. This alleviates a variety of static checker errors from code such as the following.

Yet this current justification seems rather weak, comparative to the lost potential optimization and simplicity. Furthermore, it caters to compilers instead of JavaScript itself, which many might prefer being the focus instead?

My issues with the current rationale is that, if it is claimed that "a variety of static checker errors" are alleviated, then might more be shown? Next, I believe that the point may be entirely misguided, as for example, at least TypeScript (which is commonly used, therefore having a sufficient amount of weight here) could avoid this issue by using it's function signature overloads, such that if the object argument has an insert property, then the return value would be the Map's value type, and without it, it would allow undefined in the return type. An example of this TypeScript signature might look like so:

Example ```ts // inclusive or between which property is present: both may not be undefined at once // Overload #1 declare function emplace(self: Map, key: K, handler: { insert(key: K, map: Map): V; update?(existing: V, key: K, map: Map): V }): V; // Overload #2 declare function emplace(self: Map, key: K, handler: { insert?(key: K, map: Map): V; update(existing: V, key: K, map: Map): V }): V | undefined; // overload #2: this is a compile-time type error: return includes undefined, yet `x` does not! const x: number = emplace(new Map(), "a", { update: n => 1 + n }); // overload #2: this is fine, signatures align const y: number | undefined = emplace(new Map(), "a", { update: n => 1 + n }); // overload #1: this is fine, signatures align const z: number = emplace(new Map(), "a", { insert: () => 0, update: n => 1 + n }); ```

Because an empty handler ({}) is going to have only one of the functions called, according to it's spec algorithm, thus would be erroneous, hopefully this makes it clear(er) that such a signature as given above is necessary for static-type checkers to use anyways? If so, then the current rationale should be considered invalid.

dmchurch commented 6 months ago

I came to the Issues section to raise exactly this point in exactly the way you raised it, but discovered that you'd already done so, my thanks!

Can we get a response from the champion(s) on this? Not having the single-lookup update on a method designed to be the catchall for single-lookup insert/update operations seems not only like a missed opportunity but also an unintuitive deviation from current ECMAScript standard collection behaviors; all of {__proto__: null}, String, Array, Map, and WeakMap return undefined on a failed lookup.

One other point: I would intuitively expect the following code:

const value = map.emplace(key, {});

to have exactly the same semantics as this code:

const value = map.get(key);

Here's an example use case off the top of my head in which one or both of insert and update might be missing:

function getReservationForRequest(customerId, reservationInformation = null, customerAddress = null) {
  return reservationMap.emplace(customerId, {
    // create new reservation if information is provided as a result of a checkout process, say
    insert: reservationInformation
              // new CustomerReservation() expects that address might be null, if it will be requested later in the UX flow
              ? () => new CustomerReservation(customerId, reservationInformation, customerAddress)
              : undefined,

    // update customer address on file as long as it was provided
    update: customerAddress
              // updateAddress is a fluent API, so it returns itself, which keeps the map the same
              ? reservation => reservation.updateAddress(customerAddress)
              : undefined,

  // if no entry exists, create a dummy object so that the return value of this function always knows customerId
  }) ?? new NullCustomerRegistration(customerId);
}

A call that lacks both reservation information and customer address (like, say, on a customer's "Registration Status" page) should have the effect of fetching an existing map entry (reservation) if one exists, and reporting that it is missing if it doesn't exist by returning undefined.

dminor commented 1 month ago

The new design is focusing on just the insertion use case, so the handler would always be present.