Skip to content
This repository has been archived by the owner on Nov 27, 2023. It is now read-only.

Subclassing #58

Closed
zloirock opened this issue May 28, 2023 · 7 comments
Closed

Subclassing #58

zloirock opened this issue May 28, 2023 · 7 comments

Comments

@zloirock
Copy link
Contributor

zloirock commented May 28, 2023

class MyMap extends Map { /* ... */ }
MyMap.groupBy(something, fn) instanceof MyMap; // -> false

image
Why? Object always was a special case, more other, Object.groupBy should create an object with null prototype. But Map... Unlike some new instance methods where TC39 decided to break subclassing, new static methods use proper subclassing and create new instances from this. Promise.any, Array.fromAsync, Promise.withResolvers, etc...

@bathos
Copy link

bathos commented May 28, 2023

The current algorithm has no [UC] interleaving after the generic portion — it initializes [[MapData]] unobservably. That seems pretty valuable given we can trivially shadow groupBy in a subclass when needed, but if %Map.groupBy% delegated to “public API” by default, wouldn’t it mean non subclass usage pays a “global” perf penalty? I agree it’s a departure, but it looks like this is a healthier model going forward.

@zloirock
Copy link
Contributor Author

Even new Map([[a, b])) calls this.set inside for each pair instead of working with [[MapData]]. All such cases can be optimized with protectors.

@ljharb
Copy link
Member

ljharb commented May 28, 2023

The committee hasn’t seemed interested in furthering those patterns on any new methods. The Promise statics were this way solely to remain consistent with Promise.all and race (and fromAsync with from). I would expect that withResolvers - which is not stage 3 and thus nowhere near complete - will in fact have this proposal’s semantics.

When subclassing anything, you have no guarantees methods will “just work”, and there’s nothing wrong with reimplementing it on a subclass yourself.

@bakkot
Copy link
Contributor

bakkot commented May 28, 2023

See (brief) discussion here as well as earlier on Matrix (and the next couple days after that link).

Even new Map([[a, b])) calls this.set inside for each pair instead of working with [[MapData]].

In reviewing subclasses of Set/Map in the wild, I became convinced that this was a mistake. People who would benefit from it don't know to rely on it (x, x) and other people are just straight-up broken by it (x, x).

I think this is pretty strong evidence that our attempts to add affordances for subclassing in the past have been too clever by half, and we should just let people write subclasses which customize behavior of methods by replacing those methods (you know, the normal way subclassing works), instead of trying to provide customization points.

@zloirock
Copy link
Contributor Author

zloirock commented May 29, 2023

It's very sad that someone from the committee wanna kill good practices added in ES6 and throw the language back to the last millennium.

In reviewing subclasses of Set/Map in the wild, I became convinced that this was a mistake.

In this case - maybe, it was just a performance-related example.

People who would benefit from it don't know to rely on it and other people are just straight-up broken by it.

However, a significant part of the Web has benefited from it. For example, patching those methods also fixes the constructor without patching it where it's not required. Or delegation to .then in methods allows to fix all possible built-in APIs and Promise methods without patching all of them. In many cases, after a full replacement of a built-in constructor, core-js reuse original methods that work by duck-typing API. It's only a couple of many similar examples from my practice - and, as I wrote, a significant part of the Web has benefited from it.

Personally, I can live without it - it's much simpler to use polyfill implementation instead of reuse of native features, however, users will suffer because of extra problems.

@michaelficarra
Copy link
Member

@zloirock You can read more about the committee's opinions on subclassing built-ins in the 31 March 2022 notes and accompanying slides.

tl;dr: Users shouldn't sub-class built-ins, they should wrap them instead. Use a public protocol for function arguments, but manipulate private fields (or internal slots) directly for the this value. Don't respect Symbol.species in new places, it was a mistake.

@ljharb
Copy link
Member

ljharb commented May 30, 2023

Closing as answered.

@ljharb ljharb closed this as completed May 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants