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

Introduce AbstractMap and AbstractSet for real. #6

Open
erights opened this issue Nov 17, 2019 · 22 comments
Open

Introduce AbstractMap and AbstractSet for real. #6

erights opened this issue Nov 17, 2019 · 22 comments

Comments

@erights
Copy link
Collaborator

erights commented Nov 17, 2019

During the tc39 post-presentation discussion, it was suggested that AbstractMap and AbstractSet be introduced as actually "superclasses", not just spec fictions, above Map and Set as well as ReadOnlyMap and ReadOnlySet. IOW, Map.prototype and ReadOnlyMap.prototype would both inherit directly from AbstractMap.prototype. FixedMap.prototype would continue to inherit directly from ReadOnlyView.prototype, and therefore indirectly from AbstractMap.prototype. All the map query methods would be on AbstractMap.prototype. Neither ReadOnlyMap.prototypenorFixedMap.prototypewould add any new methods to those they inherit.Map.prototype` would add the update operations.

I had avoided this in the initial proposal because this is the one change that would potentially break old code. However, @ljharb suggested that, if we also left the query methods on Map.prototype itself as well as on AbstractMap.prototype, then the only observable difference from old code is the splicing of AbstractMap.prototype in the inheritance chain between Map.prototype and Object.prototype. If this doesn't break too much old code, it is indeed attractive.

Likewise of course for the other existing collections.

@zloirock
Copy link

It's not a good idea.

It will break too much code. Some code relies on existent collections prototype chain, some code check existence of own methods on collections prototypes (for example, this check works for all methods in core-js).

For consistency with these conceptions, we should have AbstractArray, AbstractTypedArray, etc. And it will definitely break too much existent code - too much code relying on the current Array prototype chain and that Array methods are own methods of Array.prototype. We will have possible breakage with the rest collections too - the first example with DataView from core-js.

We already have Weak(Map|Set) collections - their methods are a subset of Map|Set methods, but they haven't abstract superclasses - their methods have different internal logic, here the same situation. We can't use Set#map on ReadOnlySet since this method should internally use a direct call of .add method, but read-only collections are read-only.

@erights
Copy link
Collaborator Author

erights commented Nov 18, 2019

the first example with DataView from core-js.

Thanks for the counter-example! It looks valid.

@ljharb , do you find this a convincing counter-example? Should we return to our original plan where the Abstract* classes were just an expository fiction and would not actually appear in the proposed spec?

@erights
Copy link
Collaborator Author

erights commented Nov 18, 2019

My personal preference is that we proceed with the surgery that @ljharb proposed. It would make for a much cleaner system. But if that isn't practical, I'm perfectly happy to retreat to our original position.

Re #5 , if we retreat, then we'd duplicate the new query methods in the same places we duplicate the old query methods: Map.prototype and ReadOnlyMap.prototype, with FixedMap.prototype inheriting from ReadOnlyMap.prototype. We'd place new update methods where we have existing update methods, on Map.prototype alone. Likewise for the other collection. This should safely avoid disrupting any old code.

@ljharb
Copy link
Member

ljharb commented Nov 18, 2019

That example is for DataView; @zloirock, do you use the same pattern with Map and Set? I don't see it, looking through the code.

@erights this is a convincing example of the pattern - but we'd need concrete examples of the pattern being used for Map, Set, WeakMap, and/or WeakSet, otherwise I think it's worth the effort.

@zloirock
Copy link

zloirock commented Nov 18, 2019

That example is for DataView

So you really think that adding those abstractions only to those collections when this proposal is much wider is a good idea?

do you use the same pattern with Map and Set?

@ljharb I wrote that core-js check the existence of all required own methods on collection prototypes (seems it's simplified in the current version) - it's another pattern - and it also will cause breakage. And it's not only core-js problems - I saw similar checks in other libraries.

@ljharb
Copy link
Member

ljharb commented Nov 18, 2019

The plan here is to duplicate all the methods so that Map.prototype's own properties would not change, only its prototype chain.

Yes, improving anything is a good idea even if everything can't be improved.

@zloirock
Copy link

However, this is not an improvement, it's a complication. See my first comment, mainly the last paragraph.

@ljharb
Copy link
Member

ljharb commented Nov 18, 2019

I don't agree with that comment. Map and Set should have a common superclass, as should WeakMap and WeakSet. It's fine if the weak and non-weak collections aren't part of the same hierarchy.

@zloirock
Copy link

But, even if you don't agree, it will break the web.

@ljharb
Copy link
Member

ljharb commented Nov 18, 2019

That's what I'm looking for concrete evidence for; your DataView example isn't that. Can you show some?

(to be clear - code that will break if Map.prototype or Set.prototype does not inherit from Object.prototype)

@zloirock
Copy link

I wrote that core-js check the existence of all required own methods on collection prototypes

@zloirock
Copy link

zloirock commented Nov 18, 2019

BTW why Map and ReadOnlyMap should have one superclass, but Array and ReadOnlyArray - not? It's inconsistency in the scope of one proposal, it's a complication of the language, so it's definitely a bad idea.

@ljharb
Copy link
Member

ljharb commented Nov 18, 2019

@zloirock

I wrote that core-js check the existence of all required own methods on collection prototypes

and I wrote that Object.getOwnPropertyDescriptors(Map.prototype) will be identical after the proposed change here - iow, Map.prototype.hasOwnProperty('has') && AbstractMap.prototype.hasOwnProperty('has') && Map.prototype.has === AbstractMap.prototype.has will be true. Code could only be broken if it's explicitly checking that Map.prototype.__proto__ === Object.prototype in some form.

@zloirock
Copy link

So, in this case, why add an unnecessary possible case of breakage which anyway can't be implemented for the rest collections like Array or DataView? It's just an unnecessary complication.

@ljharb
Copy link
Member

ljharb commented Nov 18, 2019

Just because we can't fix everything doesn't mean we shouldn't try to fix anything.

DataView and Array aren't the same kind of collection as Map and Set - the latter are the more important ones to improve. (There's a bunch of proposals to add methods to Map and Set and Array, and none to DataView, for example - and that's been totally fine so far)

@zloirock
Copy link

zloirock commented Nov 18, 2019

Just because we can't fix everything doesn't mean we shouldn't try to fix anything. ... DataView and Array aren't the same kind of collection as Map and Set - the latter are the more important ones to improve.

I disagree.

and none to DataView

At least:

@ljharb
Copy link
Member

ljharb commented Nov 18, 2019

Those are for specific types; they're not general collection methods for any type.

@zloirock
Copy link

This does not matter.

@erights
Copy link
Collaborator Author

erights commented Nov 19, 2019

I agree that we need uniformity across all collection that this proposal applies to. I agree that such uniformity is more important than the benefits of introducing AbstractMap and AbstractSet as one offs. If we can't uniformly introduce the AbstractFoo thing to all these collections, we should introduce it to none.

I like the point about how WeakMap and Map share API, but it doesn't bother anyone that there's no common superclass. JavaScript is duck typed. The duck typing subtype relationships matter. The implementation inheritance relationships much less so.

@theScottyJam
Copy link

I also agree.

Inheritance seems like the wrong tool for the job. Right now, there's a clean hierarchy we can construct to show the relationship between Map, ReadOnlyMap, and FixedMap, but there's no guarantee it'll stay that way. I know this is contrived, but what if, one day we decide to add a write-only view, where you can only add new values to a map, but you can read what's currently there? How would that fit into the hierarchy? The issue is that FixedMap and ReadOnlyMap are defined as a subset of Map's functionality, but there's other subsets of functionality we could create from Map.

Inheritance is the wrong tool for this job. What we're really wanting here is an interface - a feature Javascript does not have, and something that should not be emulated with inheritance. Instead, as @erights mentioned, we should just document this fictional interface and let users rely on duck-typing. If something more robust is wanted, a user can use typescript, where they can have a real interface to model the relationship between Map, FixedMap, and ReadOnlyMap.

@SamB
Copy link

SamB commented Aug 7, 2023

@zloirock ... what would an AbstractView do???

@zloirock
Copy link

zloirock commented Aug 7, 2023

@SamB for example, expose common methods / accessors for ArrayBuffer views? Like .buffer, .byteLength, or .byteOffset.

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

5 participants