Closed benlesh closed 2 years ago
The issue with the name “concat” is that it comes along with Array concat’s IsConcatSpreadable behavior, which i don’t think anyone wants to see spread elsewhere.
@ljharb it doesn't seem to be that big of an issue though. Most people don't even know that exists. And concat is a nice, specific, well-understood name.
This is the most sensible solution that I have seen. Really like that it builds on the already existing concat method, should help with education.
[ [‘a’], [‘b’], [‘c’] ].concatAll()
===
[‘a’].concat([‘b’]).concat([‘c’])
Does what it says on the tin
@acutmore that's the problem tho; it wouldn't work the same for NodeLists.
This is a neat idea, but I'd worry a lot about the difference between .concat
and .concatAll
. There's already a very similar precedent in .querySelector
/.querySelectorAll
, and it does not seem to me like this follows that precedent. (I'm not sure how it could, but that doesn't really matter.)
very similar precedent in .querySelector/.querySelectorAll
Which are DOM APIs that select items, this is a JavaScript API that transforms an array. I'm not convinced that querySelectorAll is at all relevant.
that's the problem tho; it wouldn't work the same for NodeLists.
Why not make it work the same for NodeLists, et al? Why can't this also account for isConcatSpreadable
? It's basically concatenating, whether you call it "flatten" or "smoosh" or anything you like. The result is clearly a concatenated array.
The committee consensus was that this proposal could not proceed unless it did not care about IsConcatSpreadable, which is why it was changed to checking IsArray. Any part of the name “concat” is imo not an option.
The committee consensus was that this proposal could not proceed unless it did not care about IsConcatSpreadable
Context?
It seems unreasonable given that what this method actually does is concatenate things, where flat
and flatten
are entirely too vague. Especially unreasonable given that Symbol.isConcatSpreadable is so little known and used, I can't see it mattering much.
The PR is #38.
As for “little used”, standards can’t ignore any edge case, no matter how small - that’s why that symbol exists in the first place.
Why does it have to be based on flatMap
, I believe that the method name that most of us doing functional programming with containers like these is chain
...
While flatMap
tells you "mechanically" what is happening, chain
actually speaks to what it is doing for us, it is chaining or sequencing the effect of the container.
Most of the functional community (people who use these operations on MANY types) have settled on the FL spec. Also there is no conflict to be reconciled by using chain
.
So I propose do what you think is needed to alleviate the naming 🌽cern with flatten
, but when it comes to monadic operations, we should really go with the community on this one.
Just my 2 cents.
EDIT: This is more an argument against using flatMap
as proposed.
Actually after all that it looks like there is no 🌽straint on the mapping function to return an Array
so maybe it is best that it not be named chain
since it does not meet the requirements as it has to be a more general use case. So I withdraw my opinion of it being named chain
.
EDIT: Also as Array
is going to be the only Monad
that most non-FP users are going to encounter, understanding things like applying effects of a type to underlying values may not make much sense, so in that case flatMap
makes sense.
@acutmore that's the problem tho; it wouldn't work the same for NodeLists.
The issue with the name “concat” is that it comes along with Array concat’s IsConcatSpreadable behavior, which i don’t think anyone wants to see spread elsewhere.
@ljharb Please could you explain why IsConcatSpreadable would cause an issue?
I have only just come across that Symbol so not aware of the reasons why it should be avoided. I can see how adding that extra check will reduce performance but does give people a way to opt-in to flattening array-like objects.
TC39 reps blocked this proposal as long as it had that check; I’ll let them speak up here if they wish to explain their reasons, but that is nonetheless the situation.
This proposal is stage 4, with flat
and flatMap
.
TL;DR: Try
concatAll
andconcatMap
because they're more specific to the actual behavior thanflatten
andflatMap
, because "flat" has a more general meaning. Also, there's popular precedent in RxRxJS has a precedent here, as
concatMap
andconcatAll
do the same thing in RxJS as what is proposed here, whereflatMap
(which changed tomergeMap
in v5 because it's too ambiguous as to how it's "flattening") does something very different and very specific to observing pushed values.Specifically this is the behavior of
concatMap
andconcatAll
in RxJS:concatMap: (roughly speaking) maps to a new observables, and plays them in a serial fashion, one at a time, end to end.
concatAll: Takes a higher-order Observable (Observable<Observable>), and queues up each inner observable, playing them in a serial fashion, one at a time, end to end.
As you can see,
concatMap
is more likeArray.prototype.flatMap
(proposed) than RxJS'sflatMap
is. "flat" doesn't makes as much sense in Observable-land because there is more than two dimensions to deal choose to flatten, there's also time. With an array, there, could only be two dimensions, the length of the array, and the depth. Because of that ambiguity, RxJS opted to go with something more specific and moved away fromflatMap
and named itmergeMap
(which as you can see above is very different from theflatMap
proposed here).It's just a proposal as an alternative to "smoosh".
I feel like I've rambled, but I hope that gets the idea across.