RFC: Should we import-gate flatMap?

[raquo]

With Airstream observables, flatMap has very predictable behaviour when used as intended, but can cause unexpected behaviour when it's incorrectly used as a replacement for combineWith. (See https://github.com/raquo/Airstream/#flattening-observables). The problem only occurs when you run into FRP glitches – basically if your flatMap's inner and outer observables "emit at the same time" (have a mutual and synchronous dependency). Which on one hand is good because the scope of the problem is limited, but on the other hand is bad because you can get used to incorrectly using flatMap with no issues until suddenly flatMap just does not seem to work right in one case, and if you don't know better, that feels like a bug.

It's also worth noting that Scala's for-comprehensions syntax uses flatMap behind the scenes, so users might even be using the flatMap operator without thinking about it. (For this reason, you should not use Scala's for-comprehensions with Airstream observables, they will likely produce undesired output)

---

As suggested on Discord:

I fear most of the problem is just that some users (including me) didn't read the documentation carefully and it is really their (my) fault. Making flatMap available only after some special import would probably help, but this is a highly invasive solution for a problem I assume most don't face because they've read the docs.

like 90% of my flatMaps were overkill ... [meaning, should have used combineWith instead]. After I changed my code only very few flatMaps were left and I'd be okay with needing some import for that.

But the main reason I'd appreciate it is that the kind of errors misusing flatMap produces are really hard to debug and figure out. It feels like the kind of errors that will let you drop a library/a way of doing things completely.

Laminar already import-gates : => Unit sinks because it can be dangerous if used accidentally. Basically, if you try to use that feature, you'll get a compilation error with a text suggesting you read the section of documentation linked above, which tells you to import a special implicit value to allow such syntax.

However, flatMap is an established feature (unit sinks were import-gated as soon as they were added), and also probably a more popular feature than unit arrows (even discarding incorrect uses of flatMap), so import-gating it might cause more annoyance than it's worth.

On the other hand, preventing users from getting into predictable WTF situations is pretty high on my list of design goals for Laminar & Airstream, so I need to consider various solutions for this. I do like relying on implicitNotFound messages (similar to unit sinks) because they tell you exactly what you need to know, and you can "dismiss" the error with a single import, which can be easily done either as-needed with one keystroke (in IntelliJ at least), or automatically as part of the Scala file template in the IDE.

One could say that this issue just needs better documentation instead, but it's already explained in the very first paragraph in the "flattening observables" documentation section linked above, and I think the reason this is not enough is because people generally don't expect to need documentation just to use flatMap, so they don't look it up prior to use, so I'm not sure that more documentation would actually help.

Thoughts and comments are welcome!

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

[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

[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]

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

[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]

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.