tc39 / proposal-flatMap

proposal for flatten and flatMap on arrays
https://tc39.github.io/proposal-flatMap
214 stars 19 forks source link

isConcatSpreadable implications for DOM collections #34

Closed domenic closed 6 years ago

domenic commented 7 years ago

This seems a bit unfortunate:

const divs = document.querySelectorAll("div");
const spans = document.querySelectorAll("span");

const divsAndSpans = [divs, spans].flatten();

In current proposal, divsAndSpans is a two-element array [divs, spans], not an array of all divs and spans.

Furthermore, my understanding is that DOM collections cannot implement isConcatSpreadable, because doing so is web-incompatible (with existing uses of .concat()).

My tendency would be to introduce yet another symbol/protocol for "flattenable".

ljharb commented 7 years ago

Would arrays get a default Symbol.flatten that defers to isConcatSpreadable, or would the lack of Symbol.flatten fall back to isConcatSpreadable, or something else?

domenic commented 7 years ago

I was thinking we'd add it to arrays. However I'm not sure it's time yet to focus on my particular solution; I kind of regret throwing out out there in the last sentence. First perhaps we should explore the problem space and ensure everyone agrees on the desired outcome.

pitaj commented 7 years ago

Doesn't the iterator protocol essentially say "hey look I've got some elements", which is an analog to saying, "hey look I can be flattened". I say, either flatten only Arrays, or all iterables. There's no need for another protocol which really doesn't have any different semantics.

Edit:

If you disagree, can you provide a use case for either an iterable that can't be flattened, or a flattenable object that can't be iterated?

The only thing I can think of is maybe a Map should flatten with values instead of entries, but that's really case-by-case.

bakkot commented 7 years ago

@pitaj, the more general case of iterable vs isConcatSpreadable is discussed in #8.

pitaj commented 7 years ago

Strings are a very good counterexample, thanks. As it is, isConcatSpreadable is essentially the same as only Arrays, so I have no qualms.

michaelficarra commented 7 years ago

I received some pretty wise advice from one of the TC39 delegates during my presentation: "It's Array.prototype.flatMap after all". As @pitaj says, isConcatSpreadable essentially means arrays. I think it's acceptable to ask someone who would like to defer to another protocol (such as Iterable) to use flatMap to expand it before flattening:

listOfNodeLists.flatMap(x => [...x])

So I'm leaning toward sticking with isConcatSpreadables.

Jamesernator commented 7 years ago

Given that the most common argument against Array.from behavior is strings, could it be the case that it does use Array.from, but there's an additional symbol like Symbol.isFlattenable which defaults to true if the property doesn't exist, but if it does exist and is false then it won't flatten.

e.g.:

const notFlattenable = {
    [Symbol.isFlattenable]: false,
    *[Symbol.iterator]() {
        yield* [1,2,3,4]
    }
}

const flattenable = {
    *[Symbol.iterator]() {
        yield* [5,6,7,8]
    }
}

[notFlattenable, flattenable, 'foo'].flatten()
// -> [notFlattenable, 5,6,7,8, 'foo']
domenic commented 7 years ago

At TC39 @wycats said (paraphrasing from memory, apologies if I misinterpret) that he doesn't expect people to have any good intuition about mixed-depth cases. As such, it would be OK if they get "unexpected" results where strings get flattened into their component characters, in cases like ["foo", ["bar"]]. As long as [["foo"], ["bar"]] goes to ["foo", "bar"], the mixed-depth case could be sacrificed.

I'm not sure how that would work in terms of a mechanism, but maybe it gives us some extra leeway in the problem space at least.

ljharb commented 7 years ago

I don't think that's reasonable; I would absolutely expect ['foo', ['bar']].flatten(Infinity) to remove all arrays only.

wycats commented 7 years ago

@domenic I think that's about right. Let me try to reconstruct my thinking...

I think people might have one of two crisp intuitions:

I'm personally somewhat worried that in our effort to support a more general array-like or iteration protocol, we're losing a crisp intuition that would apply to JSON and literal cases. To me, the common use-cases involve loops (or loop-like functions) that end up producing mixed-nesting literal arrays (or similar JSON) and not so much arbitrary iterables, which seem to have unclear semantics.

I share @ljharb's sensibilities about the example he posted.

Maybe one area to explore: perhaps we could allow a predicate (even from a fixed list of predicates) that could be used to decide on the flattening rule?

pitaj commented 7 years ago

[].flatten should either flatten only Arrays or all iterators, and nothing in-between. In my opinion, adding yet another protocol is unnecessary and would only serve as more bloat.

arr.flatten(1) should act like Array.prototype.concat.apply([], arr), which means the first option, via isConcatSpreadable.

domenic commented 7 years ago

I just don't feel comfortable proceeding with this feature if it excludes web platform collections.

In general, although conservatism in adding new protocols is admirable, I think we just don't have the vocabulary we need yet in ECMAScript. We can't lump everything under the heading of "is an array" or "is iterable" or "is concat-spreadable". There are legitimately more possible characteristics than those three in the space of collections, and I think "flattenable" is a potentially-interesting characteristic which we currently don't have a way to express.

"is concat-spreadable" is a particularly-bad foundation to be building upon. As the name implies, it's meant for a specific method (concat), and not to be generally used. Remember, it was designed as a legacy hack so that DOM collections could derive from arrays but be not-concat-spreadable for web compat reasons. It was never meant to be a general protocol for any other purpose.

wycats commented 7 years ago

@domenic I would be comfortable with "flattenable" (or flatMappable) being a first-class concept in ECMAScript if we persuaded ourselves that it could serve a wider array of purposes than this method. I believe that it does, but we should do the legwork on other extensions first.

wycats commented 7 years ago

(yes, I realize I'm treading dangerously close to religious territory here, but I mostly welcome it)

pitaj commented 7 years ago

I agree with @wycats that those who propose yet another protocol should provide a use case for it that exists beyond the realm of only Array.prototype.flatten and .flatMap. They should also produce several cases demonstrating usefulness of .flatten working on more general collections.

For the record, I don't see any usefulness in having .flatten work for NodeLists as any instance of usage there I can see the spread operator being a perfect substitute a some earlier point in the code.

domenic commented 7 years ago

For the record, I don't see any usefulness in having .flatten work for NodeLists as any instance of usage there I can see the spread operator being a perfect substitute a some earlier point in the code.

In that case I suggest we also require using the spread operator for arrays.

wycats commented 7 years ago

In that case I suggest we also require using the spread operator for arrays.

Is this largely about trying to find a way to keep DOM collections as array-like as possible without making them inherit from Array?

If that's the case, I wouldn't necessarily be opposed to adding a new protocol here that is the real "array-like", and doesn't have any legacy baggage/behavior (like isConcatSpreadable). If we did that, this new protocol would be appropriate for array-like APIs, while iterables would be appropriate for syntactic forms (for/of and spread).

Can you think of any reason that the set of array-like/flattenable types wouldn't be a strict subset of iterable types? (in other words, can you think of any type that is not iterable but would want to be flattenable?)

pitaj commented 7 years ago
const divs = document.querySelectorAll("div");
const spans = document.querySelectorAll("span");

const divsAndSpans = [divs, spans].flatten();

That's your example of flatten, it may not be real-world, but my point is that pretty much every usage of .flatten including a non-array member that you want flattened can be rewritten with the iterable spread operator being used earlier up the line. Here's your example re-written:

const divs = document.querySelectorAll("div");
const spans = document.querySelectorAll("span");

const divsAndSpans = [...divs, ...spans].flatten();
wycats commented 7 years ago

@pitaj I think the issue is that we have legacy objects that for whatever reason cannot inherit directly from array, but which we want to behave almost as if they were arrays. This API would introduce an inconsistency that I believe @domenic would like to try to avoid.

@domenic Am I getting that right?

domenic commented 7 years ago

That sounds right.

pitaj commented 7 years ago

I understand exactly what you want and why. I'm saying that a new protocol is, at best, overkill for the perceived problem here. And I'm saying you can solve the same issue by simply adding a spread operator upstream.

If I'm wrong, please provide a counterexample.

domenic commented 7 years ago

You can always solve a feature not working by adding code explicitly to make it work. You are not wrong about that.

I just don't think we should accept a feature into the language that requires extra code to be made to work for some of the most common collections in all of JavaScript.

wycats commented 7 years ago

@domenic I think you must be saying something more than "it should work with common collections." For example, strings are pretty common collections, and people universally seem to think flatten shouldn't work with those.

You must be saying that you want DOM collections to be more like arrays than simple collections. I can totally get behind that point of view but I just want to be sure.

domenic commented 7 years ago

Yes, that's correct.

pitaj commented 7 years ago

What about Sets? What about Maps? What about their Weak counterparts? Do those get flattened? How would that behavior work?

DOM collections have never been treated the same as arrays, why should that start now? Creating a new protocol for a single use case at the edge of an API seems petty to me. Especially live NodeLists match the idea of an Iterator much more than an Array.

I see consistency as a huge priority in API design. So you have to choose what kind of consistency you want.

wycats commented 7 years ago

Maps aren't even iterable (without .entries() et al), so it doesn't make sense to flatten them. WeakMaps and WeakSets can't be iterated at all. Sets can be iterated, but aren't notionally ordered.

The interesting thing about DOM lists is that they really should have been arrays, but for historical accidents. It seems reasonable to consider that concern when designing new APIs that are meant to work with array-like constructs. The string example is enough evidence that "iterable" ain't the solution to all array-like cases, and DOM lists show that Array.isArray is too narrow.

At TC39, many of us thought that isConcatSpreadable was too coupled to historical idiosyncrasies of the concat API. But is that really true? Maybe it's a silly name but with the right semantics?

Edit: I missed this in the original comment:

Furthermore, my understanding is that DOM collections cannot implement isConcatSpreadable, because doing so is web-incompatible (with existing uses of .concat()).

That's a good enough reason to consider a more-compatible solution for array-like behavior. @domenic I think that really would imply that concat was dead, since the inconsistency between flatten and concat would be pretty surprising.

bakkot commented 7 years ago

@wycats, the point of this issue is that it isn't the right semantics, right? Its whole reason to exist is to exclude node lists; it differs from isArray only in including arguments objects.

ljharb commented 7 years ago

@wycats Map.prototype[Symbol.iterator] === Map.prototype.entries - Maps are definitely iterable by default. Both Maps and Sets are ordered by "insertion order".

That said, I kind of agree that isConcatSpreadable, while a crappy name, really kind of is "arraylike".

However, a Symbol.flattenable would probably be too narrow, and a Symbol.arrayLike would probably need to work with concat too :-/

wycats commented 7 years ago

@bakkot yeah I realized after the original post that I was wrong: we do need "high-fidelity array-like, but not Array.isArray and not Iterable" here if we want to get Array, arguments, and Node Lists, but not strings.

We're sort of firming up the "array-like" protocol that's used in the generic versions of the array methods, but for cases where we really do need to know that the array-like protocol wasn't an accident.

domenic commented 7 years ago

I don't understand how the idea of "flattenable" can be considered narrow when the alternative is "spreadable by the Array.prototype.concat method". The latter is about as narrow as you can get.

wycats commented 7 years ago

Map.prototype[Symbol.iterator] === Map.prototype.entries

I messed that up!

Both Maps and Sets are ordered by "insertion order".

Yeah, my point was really just that Maps and Sets, while defined in terms of insertion order, are less intuitively ordered, so the question of what the "obvious" behavior with those collections is not as obvious as the DOM Lists, which really try very hard to behave like arrays.

One check: does the collection implement length and integer-indexed keys? If not, it's probably not a great fit for this design. If yes, it probably is (but might not be: the protocol might have been implemented by accident, and also strings).

ljharb commented 7 years ago

@domenic lol, that's true, and I don't object to Symbol.flattenable at all - but I don't see how it'd be much different than isConcatSpreadable, except solely for NodeLists, which aren't concat spreadable but would be flattenable - it seems unfortunate to again add a protocol to the language for no reason but NodeLists.

wycats commented 7 years ago

I don't understand how the idea of "flattenable" can be considered narrow when the alternative is "spreadable by the Array.prototype.concat method". The latter is about as narrow as you can get.

Agreed. I think the name "flattenable" might be causing it to feel narrower than it is. Maybe something more like Symbol.isArrayLike? I think it would be acceptable to be inconsistent with concat if we're willing to call concat a historical wart with too much baggage to worry about.

ljharb commented 7 years ago

It's weird tho that a NodeList would be arraylike, but then not spreadable by concat :-/

wycats commented 7 years ago

@domenic lol, that's true, and I don't object to Symbol.flattenable at all - but I don't see how it'd be much different than isConcatSpreadable, except solely for NodeLists, which aren't concat spreadable but would be flattenable - it seems unfortunate to again add a protocol to the language for no reason but NodeLists.

There are other array-like objects that probably have the same compat baggage relative to concat but might be able to opt into new protocols for new APIs. jQuery is a (very popular) example.

wycats commented 7 years ago

It's weird tho that a NodeList would be arraylike, but then not spreadable by concat :-/

Yeah, I think any of these proposals would have to give up on concat feeling consistent. That's kind of already true though.

domenic commented 7 years ago

We have a pretty long history of single-method-affecting protocols. In addition to isConcatSpreadable, we have match, replace, search, and split. Plus a number of symbols only impact a single syntactic form (hasInstance, unscopeables, ...). I'm not sure how general we need to be here: something that impacts both flatten and flatMap is already used by 2x as many methods as most symbols are.

In general it seems likely that people will want to create collections that are like string (iterable, not flattenable, not concat-spreadable) and like array (iterable, flattenable, concat-spreadable) and like NodeList (iterable, flattenable, not concat-spreadable). Future methods may also need their own symbols, if they have plausible use cases for both behaviors that are orthogonal to their behaviors with flatten and concat and the iteration protocol. I don't think we'll be able to forever divide the universe of collections into two or three categories (string, array, NodeList), but instead will likely have more as collections gain more interesting axes.

wycats commented 7 years ago

@domenic Don't you think that the main reason for the NodeList situation is that concat() is already an API, so changing NodeList to behave differently with concat would be a breaking change?

In other words, if concat() was a brand new API, wouldn't we want it to be consistent with flatten?

domenic commented 7 years ago

I'm not sure. I don't have a strong intuition about concatenating arrays and other collections. For example, [].concat(new Set([1, 2, 3])) gives [theSet], not [1, 2, 3]. So is it surprising that when you do [].concat(divs), you get [divs], not [divA, divB, divC]? Or is it expected?

On the other hand I do have a pretty strong intuition about the OP (namely that it should end up with an array of nodes, not an array of NodeLists). So my intuitions definitely vary about whether NodeLists should be concat-spreadable (unsure) versus flattenable (definitely).

Similarly I am unsure about Sets: concat-spreadable (probably not) versus flattenable (probably?).

wycats commented 7 years ago

@domenic I think I mostly agree with your categorizations. I personally have a stronger hunch that the reason I have a weak intuition around concat is that it's an API with a lot of backwards-compatibility edges and is inherently inconsistent.

I share your intuition about NodeLists (flattenable, definitely) but not about Sets. I think it has to do with the programming patterns that lead you to have nested collections in the first place.

["div", "span", "p"].map(selector => document.querySelectorAll(selector)).flatten();

In these cases, the platform has worked hard to make querySelectorAll return something that feels like an array, and in fact the only ergonomic way to use these APIs gives you back a DOM sequence, not an Array. In other words, in the DOM, DOM sequence is the Array type. In this context, treating them as different from Arrays is generally a bad thing, and generally violates user intuition.

In contrast, Sets are generally used, in APIs, to represent something different from "an ordered sequence of items" (even though, as @ljharb pointed out, they're specced to be ordered). So you're less likely to end up in a situation where you have mixed arrays and sets and want them to be combined into a flattened array.

[EDIT] My general intuition here is that when there is broad agreement that you intuitively expect a particular sequence to flatten, they should flatten, but if there are unclear intuitions, we should err on the side of not flattening. To me, this seems likely to produce a clearer set of expectations.

On the other hand, it could be quite useful to flatten Sets of Sets, but that would imply flatMap and flatten on Set rather than making Arrays of Sets flatten them.

michaelficarra commented 6 years ago

Fixed by #38.