search

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

Class extension/what is primitive #49

Closed conartist6 closed 2 years ago

conartist6 commented 2 years ago

The proposed API has significant problems for class extension. I'd argue that this is because Map is actually a primitive type, and upsert is a very high-level operation that does not belong on such a primitive type. I know that primitive and high level types are an extremely fuzzy distinction in JS, mostly because of Array, which is both the lowest level and highest level construct in the language. I thought we were determined not to repeat the mistakes of Array -- to have low level primitives be distinct from high level functionality.

For one thing, Map isn't just extensible -- it's designed to be extensible, or even usable as an interface. Just a few days ago I created a class for grammar productions. It implements the Map interface, except that productions.get('Literal') might return productions.map.get('Node'). Rules about nodes apply to literals.

If this proposal were to pass, I'd have to implement productions.upsert (or I would if my structure were mutable, which, it could be). The likelihood of my implementing an operation as complicated as upsert correctly are low.

ljharb commented 2 years ago

How is upsert a high-level operation? It's largely a shorthand for if (!has) { set } return get, which should be operations that apply to any Map subclass.

ljharb commented 2 years ago

And yes, the addition of any new method to base classes may require your subclass to implement it as well - this is just how JS works.

conartist6 commented 2 years ago

I mean, my thought is to have set(key, getOr(key, defaultValue)). I could implement getOr in one line, and it'd be pretty hard to mess up. I'd guess that emplace is about 20 lines to implement correctly in a script, which to me means that nobody should ever do it themselves.

ljharb commented 2 years ago

anything with defaultValue doesn't cover the use case where defaultValue is expensive to compute, which is a primary motivator of this proposal.

conartist6 commented 2 years ago

Ah, true. It could still trivially be value => defaultValue though.

ljharb commented 2 years ago

Sure - and then you're at the original design of this proposal, which additional constraints caused to change to look like it does now.

conartist6 commented 2 years ago

Hmmmmmm

papb commented 2 years ago

I'd have to implement productions.upsert (or I would if my structure were mutable, which, it could be). The likelihood of my implementing an operation as complicated as upsert correctly are low.

But would you? Can't your subclass just inherit the base implementation of upsert and have it just work automatically, due to polymorphism, assuming that you've already overwritten get, set and has accordingly?

conartist6 commented 2 years ago

No because emplace does not actually call get set and has for perf reasons. Extenders of Map would have to also override upsert, and it would be important to define the override of upsert in terms of super.upsert not in terms of this.get, this.set, and this.has, otherwise the semantics would be non-spec.

Implementers of the Map "interface" would also need to be careful to avoid implementing upsert in terms of get, set, and has, but they should be able to do this since they would have access to the encapsulated data.

conartist6 commented 2 years ago

Ah but that is a problem

papb commented 2 years ago

No because emplace does not actually call get set and has for perf reasons.

Oh, I didn't know, sorry about that. Where did you get this information? At least the current core-js polyfill is polymorphic on get, set and has.

conartist6 commented 2 years ago

I was going by the README. Also see https://github.com/tc39/proposal-upsert/issues/47