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

Class extension/what is primitive #49

Closed
conartist6 opened this issue Nov 23, 2022 · 12 comments
Closed

Class extension/what is primitive #49

conartist6 opened this issue Nov 23, 2022 · 12 comments

Comments

@conartist6
Copy link

conartist6 commented Nov 23, 2022

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
Copy link
Member

ljharb commented Nov 24, 2022

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
Copy link
Member

ljharb commented Nov 24, 2022

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
Copy link
Author

conartist6 commented Nov 24, 2022

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
Copy link
Member

ljharb commented Nov 24, 2022

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

@conartist6
Copy link
Author

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

@ljharb
Copy link
Member

ljharb commented Nov 24, 2022

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
Copy link
Author

Hmmmmmm

@papb
Copy link

papb commented Nov 25, 2022

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
Copy link
Author

conartist6 commented Nov 25, 2022

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
Copy link
Author

Ah but that is a problem

@papb
Copy link

papb commented Nov 25, 2022

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
Copy link
Author

I was going by the README. Also see #47

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants