RFC: Should we import-gate flatMap? about airstream

[yurique]

A couple of random thoughts:

• I don't think I ever needed flatMap on signals
• whenever I use them on streams, it's legit (but I'm always looking for alternatives before doing flatMaps because I don't like them :) )
• stream.flatMap(x => signal) is probably always a bad idea

Gating those could be an "easy" decision?

Random idea: we could import-gate the flatMap, but also provide another method which will do the same flatMap without requiring an import?

In akka streams they use flatMapConcat and flatMapMerge (and flatMap doesn't exist) (https://doc.akka.io/docs/akka/current/stream/operators/Source-or-Flow/flatMapConcat.html, https://doc.akka.io/docs/akka/current/stream/operators/Source-or-Flow/flatMapMerge.html)

Maybe we even deprecate (and then remove) flatMaps?

(this will probably also help prevent incorrect for-comps)

[raquo]

Aight, this is what I implemented so far:

• flatMap and flatten method usages will start failing at compile time with an informative error

  • to help migration, for now, these errors can be replaced with a deprecation warning by importing a special implicit
    • later this option will go away
  • unless you manually used ConcurrentStreamStrategy, then you will need to use the new flatMapMerge.

• new operators are flatMapSwitch (behaves like the old flatMap with no strategy provided), flatMapMerge, flatMapCustom (if you want to provide your own FlattenStrategy), as well as the flatten variants of the same: flattenSwitch, flattenMerge, flattenCustom.

  • Not a big fan of those flatten names tbh, going for consistency with flatMap names. I was also thinking about flatSwitch and flatMerge, but those are painfully arbitrary / non-consistent.

• I also added a flatMapTo(stream) alias to flatMapSwitch(_ => stream), to clean up a popular use case.

• In Laminar:

  • flatMap operator on events (e.g. onClick.flatMap(_ => stream) will remain as it is today (no warnings or errors) because it doesn't have the same problems as observables do. It will no longer support ConcurrentStreamStrategy, because that combination never made sense anyway.
  • flatMap operator on events (e.g. onClick.flatMap(_ => stream) will start using flatMapSwitch under the hood, which means no changes unless you manually provided a non-default flattenstrategy to it, in which case you will get a compiler error.
  • I also added a flatMapTo(stream) alias to flatMap(_ => stream), to clean up a popular use case.

[felher]

I'm in favor of hiding it behind an import statement for exactly the reasons mentioned in the above (my) discord quote. And also because of what @raquo said:

people generally don't expect to need documentation just to use flatMap, so they don't look it up prior to use

Also, I did a quick check and went through some of my projects. For the one mentioned on discord I already noticed that 90% of the flatMaps were not needed.

I also checked two other projects. In one of them, ~~80% percent of flatMaps were wrong, in the other about ~~70%.

I also noticed that flatMaps (and of course for-comprehensions) were much less frequent than I thought. Needing an import would not affect most of the files even in the scalajs section of the projects.

Also, one of those projects is already in production and is used by quite a few people. Which highlights one of my fears: It mostly works. But you sometimes get messages from people saying something didn't work when they used it. And often, it has nothing to do with the software, like they are using a browser from 2010 or have some weird security restrictions on their network or something. But some of them might have stumbled upon a real bug in our code caused by using flatMaps where they should not be used.

For example, I have found code like this:

for {
  value  <- node.map(_.head.value)
  spec   <- node.map(_.head.spec.data)
  result <- zipperFromSpec(spec) match {
              case None         => invalidSpec
              case Some(zipper) => doStage(value, spec, zipper, open.signal, commandBus.writer)
            }
} yield result

this worries me. This seems like it would work like 99% of the time but could sometimes lead to an odd glitch which makes doStage get non-matching value and spec arguments.

Summary

• Did I experience unexpected flatMap: yes
• How many of the flatMaps/for used where legitimate: roughly 1/4 (meaning 3/4 were bad)
• How many .flattens? I only found a single .flatten and it was legitimate.
• How anoyed would I be: Not very, given a good implicitNotFound message. Mainly because flatMaps in any form were much less frequent than I thought

[phfroidmont]

I used flatMap quite a bit (mostly to trigger ajax requests so it's mostly stream.flatMap(x => stream)) and I think every time was legitimate. I did read the whole documentation before starting to use Laminar about 3 years ago.
That said, I agree this could have been a big issue if I had missed that part of the doc and could still be if someone joined my project.

I really like yurique's idea. It is more explicit, allows for smooth migration and prevents the for-comp usage.

[HollandDM]

Just like phfroidmont, I used flatMap mostly for Ajax requests, so most of them were legit. The most complicated thing I did with flatMap was something like this:

val outputListSignal = itemListSignal
      .split(_.itemId) { case (_, _, itemSignal) =>
         itemSignal.map(veryExpensiveFunction)
      }
      .distinct
      .flatMap { outputSignals =>
         new CombineSignalN(
            JsArray.apply(outputSignals),
            _.asScalaJs.toList
         )
      }

and I encountered diamond glitch case while coming up with that.

I do think we should rename flatMap so users do not accidentally for-comp them.

[raquo]

To clarify what kind of comments I'm hoping for:

• Have you run into problems with Airstream's flatMap behaving unexpectedly, because you were expecting behaviour similar to combineWith? (or maybe for some other / unknown reason?)
• How often do you use Airstream's flatMap operator, and are you using it mostly "legitimately", or mostly as a poor man's combineWith? (see here for clarity)
• Same question but about the flatten operator.
• How annoyed would you be if you needed to import something like com.raquo.airstream.api.features.allowFlatMap to be able to use flatMap / flatten? The compiler would give you an error telling that you need this import.

Also to clarify the proposal on a technical level:

• currently, flatMap method already requires an implicit param of type FlattenStrategy.
• it's automatically defaulting to a switching strategy, which is a sane default (if you've read the docs, at least).
• the change would be to remove that default, and add an implicitNotFound compiler error message telling developers why they need to be careful, and how to import the default switching strategy.
• Or alternatively, we could create a second implicit param, just for implicitNotFound (like unit sinks), and leave the existing FlattenStrategy param and implicit param as-is, for better separation of concerns.
• I'll look more into specific implementation if/when there is enough agreement that we should proceed with this.

[raquo]

Good idea to rename flatMap variations to more specific ones. Can see this being he better alternative overall.