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

concatAll and concatMap rather than flatten and flatMap #60

Open
benlesh opened this issue Mar 9, 2018 · 13 comments
Open

concatAll and concatMap rather than flatten and flatMap #60

benlesh opened this issue Mar 9, 2018 · 13 comments

Comments

@benlesh
Copy link

@benlesh benlesh commented Mar 9, 2018

TL;DR: Try concatAll and concatMap because they're more specific to the actual behavior than flatten and flatMap, because "flat" has a more general meaning. Also, there's popular precedent in Rx

RxJS has a precedent here, as concatMap and concatAll do the same thing in RxJS as what is proposed here, where flatMap (which changed to mergeMap 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 and concatAll 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.

/**
  * A test fixture that emits the passed values from a returned 
  * Observable one at a time, roughly one second apart, and completes
  * after emitting the last value.
  */
function ob(...args) {
  return new Observable(observer => {
    let i = 0;
    const id = setInterval(() => {
      observer.next(args[i++]);
      if (i >= args.length) observer.complete();
    }, 1000);
    return () => clearInterval(id);
  });
}


const source = ob(
  ob(1, 2),
  ob(3, 4),
  ob(5, 6),
);

const source2 = ob(1, 2, 3);

const mappingFn = n => ob(n, n + 1, n + 2);

const logger = { next(result) { console.log(result); } };
/* concatAll */
source.concatAll()
  .subscribe(logger);  // 1....2....3....4....5....6

/* concatMap */
source2.concatMap(mappingFn)
  .subscribe(logger); // 1....2....3....2....3....4....3....4....5

/* mergeAll (aka flatten elsewhere) */
source.mergeAll()
  .subscribe(logger); // 1.3.5....2.4.6   (roughly)

/* flatMap (now mergeMap in Rx 5) */
source2.mergeMap(mappingFn)
  .susbcribe(logger); // 1.2.3....2.3.4....3.4.5.

As you can see, concatMap is more like Array.prototype.flatMap (proposed) than RxJS's flatMap 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 from flatMap and named it mergeMap (which as you can see above is very different from the flatMap 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.

@ljharb

This comment has been minimized.

Copy link
Member

@ljharb ljharb commented Mar 9, 2018

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.

@benlesh

This comment has been minimized.

Copy link
Author

@benlesh benlesh commented Mar 9, 2018

@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.

@acutmore

This comment has been minimized.

Copy link

@acutmore acutmore commented Mar 9, 2018

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

@ljharb

This comment has been minimized.

Copy link
Member

@ljharb ljharb commented Mar 9, 2018

@acutmore that's the problem tho; it wouldn't work the same for NodeLists.

@bakkot

This comment has been minimized.

Copy link
Contributor

@bakkot bakkot commented Mar 9, 2018

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.)

@benlesh

This comment has been minimized.

Copy link
Author

@benlesh benlesh commented Mar 9, 2018

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.

@ljharb

This comment has been minimized.

Copy link
Member

@ljharb ljharb commented Mar 9, 2018

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.

@benlesh

This comment has been minimized.

Copy link
Author

@benlesh benlesh commented Mar 9, 2018

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.

@ljharb

This comment has been minimized.

Copy link
Member

@ljharb ljharb commented Mar 9, 2018

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.

@evilsoft

This comment has been minimized.

Copy link

@evilsoft evilsoft commented Mar 10, 2018

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.

@evilsoft

This comment has been minimized.

Copy link

@evilsoft evilsoft commented Mar 10, 2018

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

This comment has been minimized.

Copy link

@acutmore acutmore commented Mar 13, 2018

@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.

@ljharb

This comment has been minimized.

Copy link
Member

@ljharb ljharb commented Mar 14, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.