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

Convert to static factory methods #47

Merged
merged 1 commit into from
May 18, 2023
Merged

Convert to static factory methods #47

merged 1 commit into from
May 18, 2023

Conversation

jridgewell
Copy link
Member

@jridgewell jridgewell commented Nov 29, 2022

This reimplements the proposal using static methods on the Object and Map constructors. There seemed to be positive support for doing so in today's plenary, allowing us to side-step web compat issues that crop up from developers using arrays as hashmaps with arbitrary key-values.

The factories are made to be generic, which means that we may not return an prototype-less object from Object.groupBy (eg, via class Foo extends Object {}; Foo.groupBy(…)), and we use the map.set method instead of directly inserting into the [[MapData]].

This has not yet reached consensus, this is just a preview for us to discuss further at the next meeting.

Co-authored-by: Justin Ridgewell <justin@ridgewell.name>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
spec.emu Outdated Show resolved Hide resolved
spec.emu Outdated Show resolved Hide resolved
spec.emu Outdated Show resolved Hide resolved
@zloirock
Copy link
Contributor

In my vision, static methods look like a good option, but in addition to Array.prototype methods.

  • Static methods could accept any iterable.
  • Prototype methods could work with array-like.

@ljharb
Copy link
Member

ljharb commented Nov 29, 2022

Array.from is a static method that accepts an iterable or arraylike; that’s the precedent we should follow.

@zloirock
Copy link
Contributor

Array.from (and the upcoming async version of this method) is the only method that works with both protocols - and it was a special case. All the rest methods that accept iterables accept only iterables.

@zloirock
Copy link
Contributor

All Promise combinators, Object.fromEntries (related to this Object.groupBy case), all collections constructors (new Map(iterable) related to this Map.groupBy case), etc.

@jridgewell
Copy link
Member Author

In my vision, static methods look like a good option, but in addition to Array.prototype methods.

I think it'll be xor. We'll either decide that we like the static methods, and only have them, or we'll decide we don't and go back to picking a new name for the prototype methods.

Array.from is a static method that accepts an iterable or arraylike; that’s the precedent we should follow.

I copied Array.from, but don't think it's strictly necessary. Array.from is special in that should handle any language value so that we can get a real array instance, but I don't think groupBy needs that expectation. I think the simpler only-iterable would be fine.

@bakkot
Copy link
Contributor

bakkot commented Nov 29, 2022

The factories are made to be generic, which means that we may not return an prototype-less object from Object.groupBy (eg, via class Foo extends Object {}; Foo.groupBy(…)), and we use the map.set method instead of directly inserting into the [[MapData]].

I am ok with the Map version being generic (though I personally wouldn't bother), but the Object one should match Object.fromEntries and ignore its receiver. We should treat Object as a namespace, not a class.

@jridgewell
Copy link
Member Author

  1. Made it always return a prototype-less object
  2. Removed thisArg
  3. Removed array-like fallback

@bakkot
Copy link
Contributor

bakkot commented Nov 29, 2022

Also, for the Map fallback, I think it would be more natural to create an array of pairs and invoke the constructor with that, rather than invoking .set directly. It's more overhead, but it means it would work with "frozen" subclass which rejects calls to .set after construction, and other similar designs. (And the overhead can be skipped for the Map case as long as Map.prototype.set is intact. [edit: and the Array iterator...])

I don't feel strongly about how the Map fallback should work though.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel pretty strongly that the Object one should accept arraylikes, not just iterables (altho it's fine for Map to just accept iterables)

spec.emu Outdated Show resolved Hide resolved
spec.emu Outdated Show resolved Hide resolved
spec.emu Show resolved Hide resolved
@ljharb
Copy link
Member

ljharb commented Nov 29, 2022

I definitely agree with @bakkot's comment; it's better to make an Array and construct a Map from it rather than observably calling set N times.

@bakkot
Copy link
Contributor

bakkot commented Nov 29, 2022

I definitely agree with @bakkot's comment; it's better to make an Array and construct a Map from it rather than observably calling set N times.

Unfortunately my suggestion will still observably call set N times, because the default behavior of the Map constructor is to look up the set method and invoke it repeatedly.

Now that I think about it, it will in addition look up the array iterator and invoke that. My suggestion has strictly more observable calls, at least for the default Map constructor.

... this is definitely inclining me towards not trying to support subclassing, and instead just constructing a Map and its internal slot directly.

@bakkot
Copy link
Contributor

bakkot commented Nov 29, 2022

I feel pretty strongly that the Object one should accept arraylikes, not just iterables (altho it's fine for Map to just accept iterables)

Strong disagree. Note that Object.fromEntries doesn't accept arraylikes.

@ljharb
Copy link
Member

ljharb commented Nov 29, 2022

Rereading tc39/proposal-object-from-entries#9 and tc39/proposal-setmap-offrom#3, I'm not sure why I didn't push harder for it to do so, because there's not really a compelling argument there that i see.

@jridgewell
Copy link
Member Author

Rereading tc39/proposal-object-from-entries#9 and tc39/proposal-setmap-offrom#3, I'm not sure why I didn't push harder for it to do so, because there's not really a compelling argument there that i see.

I feel the same way as tc39/proposal-setmap-offrom#3. Array.from should accept array-likes, because something has to convert from the array-like to something iterable. Array being the de-facto iterable collection means it should be Array.from that supports it. I don't think other methods need to worry about this, it's just assumed everything is iterable.

this is definitely inclining me towards not trying to support subclassing, and instead just constructing a Map and its internal slot directly.

I think this falls under Type II, which we decided is useful?

@bakkot
Copy link
Contributor

bakkot commented Nov 29, 2022

I think this falls under Type II, which we decided is useful?

That's not what the proposal says, though of course getting rid of Type II in existing method will be fraught and I don't want to reopen that discussion.

But FWIW, for Set methods we decided that there will be no receiver-sensitivity. setSubclass.union(other) will create a bare Set, ignoring the constructor of the receiver and even bypassing the Set constructor (i.e. it will not respect a monkey-patched Set.prototype.add). That's not quite precedent, because those are prototype methods rather than statics, but it's close.

Incidentally see some discussion here and on the next few days. Sentiment seemed to be against relying on the receiver for statics.

Copy link
Member Author

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Removed Map subclass support.

spec.emu Outdated Show resolved Hide resolved
spec.emu Outdated Show resolved Hide resolved
spec.emu Show resolved Hide resolved
spec.emu Outdated Show resolved Hide resolved
@ljharb
Copy link
Member

ljharb commented Nov 29, 2022

Other than my opinion on arraylikes, this looks great to me.

spec.emu Outdated Show resolved Hide resolved
spec.emu Outdated Show resolved Hide resolved
@Josh-Cena
Copy link

Josh-Cena commented Dec 11, 2022

FWIW, I feel slightly uncomfortable about removing subclassing, especially since these methods are static.

That's not quite precedent, because those are prototype methods rather than statics, but it's close.

IIUC, generic instance methods are icky because @@species lead to security risks. But what's so hard about supporting generic static methods? Array, Promise, TypedArray all do, and, besides Object, there's none that doesn't. If throwing on a non-constructor receiver is too unergonomic, we can fall back to %Map% like Array.from does.

because the default behavior of the Map constructor is to look up the set method and invoke it repeatedly.

If a subclass (a) makes set calls observable (b) does not patch that in the constructor, it means construction of any instance is going to lead to that many observable calls, and this factory method makes it no worse from the user's perspective.

spec.emu Show resolved Hide resolved
@bakkot
Copy link
Contributor

bakkot commented Dec 11, 2022

@Josh-Cena

The Promise behavior for statics (throwing if invoked without a receiver) is pretty annoying, since it makes it harder to use on the base class, not just on subclasses. Subclassing is rare enough that I am hesitant to make the base class case worse for the benefit of subclasses.

We could match Array with the fallback, but I don't really see the benefit - a subclass would need to override it anyway so that let { group } = Foo, x = group(y, z); produces a Foo instead of a Map. This was also the sentiment of other delegates; see the discussion I linked above.

@Josh-Cena
Copy link

Josh-Cena commented Dec 11, 2022

Subclassing is rare enough

Promise subclassing could be rare; Map/Set are probably not, since things like upsert, union, etc. must have already been implemented in userland.

a subclass would need to override it anyway so that let { group } = Foo, x = group(y, z); produces a Foo instead of a Map

The fallback is just a best attempt at being useful, not that subclasses always produce consistent results as the base class. This example looks terribly like auto-bound methods; if you want to go down that path, you would probably use a getter like Intl.NumberFormat.prototype.format.

People are always going to be surprised by receivers somewhere, and fixing receiver issues these days is relatively cheap (either arrow function wrapper or bind). But if it turns out that 2% use map subclasses and 10% choose to pass Map.groupBy directly to callbacks without binding this, then those who pass MapSubclass.groupBy directly to callbacks is probably much smaller than 2% × 10% = 0.2% and I'm hesitant to say that it tangibly impacts anyone, considering it's how the language behaves everywhere else and people should already be aware of it.

What surprises me is that basically nothing works with subclassing these days; in order to make a subclass with a different primitive method (the minimal-core thing) you have to reimplement everything. That sounds like a leak of abstraction.

@ljharb
Copy link
Member

ljharb commented Dec 11, 2022

Subclassing itself isn't a well-defined concept in the language - TC39 hasn't even been able to come to consensus about what the officially supported subclassing mechanism should be - and it's just not a thing that's commonly done in the ecosystem. I'd suspect your 2% and 10% is more like 0.01% and 80%, personally.

@bakkot
Copy link
Contributor

bakkot commented Dec 11, 2022

The fallback is just a best attempt at being useful, not that subclasses always produce consistent results as the base class.

Having a method which behaves inconsistently depending on how you call it is a large cost. It's not something to be glossed over lightly.

This example looks terribly like auto-bound methods; if you want to go down that path

We do not want to go down that path. Making statics receiver-insensitive works just as well, except for subclasses, and there's no way to make subclasses work with this without auto-bound methods, which we're not going to do.

What surprises me is that basically nothing works with subclassing these days

I would put it a different way: everything works with subclassing. It's just not offering hooks for subclasses to customize the behavior of base classes. If you want to change the behavior of a method, you need to offer your own implementation of the method. That's not an abstraction leak - that's just what subclassing is.

@zloirock
Copy link
Contributor

Note that Map.groupBy is present in collections methods proposal, spec.

@bakkot
Copy link
Contributor

bakkot commented Jan 4, 2023

A downside of this approach is that half of the users will think that the order of the arguments is data, fn and half will think that it's fn, data. I am not excited to have to pick one.

I don't think we have precedent for data-first or data-last yet. The closest I can think of is with the mapper argument to Array.from, which is optional and so not really relevant.

@ljharb
Copy link
Member

ljharb commented Jan 4, 2023

Would you consider Object.create to be data-last or data-first?

@bakkot
Copy link
Contributor

bakkot commented Jan 4, 2023

Object.create doesn't take a function argument, so the question doesn't apply to it.

@zloirock
Copy link
Contributor

zloirock commented Jan 4, 2023

I'm strictly for data, fn. At least, Array.from case.

@SamB
Copy link

SamB commented Feb 25, 2023

For what it's worth, I implicitly assumed that the array-like input would be first, since these are being converted from Array.prototype methods, and my background in Python leads me to think of the method receiver (in languages where that's a thing) as a possibly-hidden first argument, which in JavaScript just happens to always be hidden.

@bricss
Copy link

bricss commented Feb 28, 2023

I've been thinking 🤔 lately, that maybe we can simply rename methods to:

  • Array.prototype.groupByKey( _callbackfn_ [ , _thisArg_ ] )
  • Array.prototype.groupByKeyToMap( _callbackfn_ [ , _thisArg_ ] )

And ship 🚀 it asap, rather then trying figure out arguments? 😀
But if someone would like to know my opinion on arguments, I truly believe ✋ it must be data, fn 🔣
Coz its very much de facto standard across whole ecosystem 📖

@ljharb ljharb force-pushed the static-method branch 2 times, most recently from 5d5b155 to 951513d Compare May 16, 2023 19:23
@ljharb
Copy link
Member

ljharb commented May 16, 2023

This PR has been rebased. Spec can be viewed here: https://raw.githack.com/tc39/proposal-array-grouping/static-method/index.html

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

Successfully merging this pull request may close these issues.

8 participants