raquo/Airstream

Airstream semantics update – RFC

raquo opened this issue · 12 comments

raquo commented

I am currently reworking Airstream behaviour in several big ways. Most of these changes will be released soon as 0.15.0, and will definitely affect your code. Please read this and provide feedback to make sure that the changes account for your use cases. If anything below sounds controversial or is devastating to your use case, please let me know. If anything sounds helpful, please let me know too. Much of this is very hard, so possibly not all of this will happen right away, so I need to know what to prioritize.

No more == checks in signals

This is a solution to #19 – signals won't compare the new value against the previous value before emitting anymore. This can break your code if you rely on signal's events being unique / deduplicated. Solution is to use the new distinct / distinctBy methods to add back uniqueness as needed. Laminar itself does not need you to do it, but you might need this to avoid making redundant Ajax requests or other side effects.

Signals will re-evaluate their current value after being re-started

This is a solution to #43.

Main caveat: only signals have a current value, so only signals that depend on other signals can do this. If your signal is stream.startWith(1), not much you can do about having missed events from stream, since streams don't replay their events for new subscribers. Same for stream.foldLeft(...)

Other caveat: there is no way SIGNAL.foldLeft(...) will fall neatly into either old or new behaviour. I will probably need to make its behaviour configurable.

Observables will stop resetting their internal state when they're stopped

For example, when stream1.combineWith(stream2) is stopped and started again, currently it won't start emitting again until both stream1 and stream2 emit a value, because it "forgets" the previous events and is returned to an empty state similar to how it was originally created. That was the old idea, and I think we're done with it now.

Similarly, when a streamOfStreams.flatten is stopped, it "forgets" which stream it was supposed to mirror, so when it starts up again it does nothing until streamOfStreams emits again.

Things like this really mess with your ability to re-mount Laminar components. The idea with all these changes is that with the new default behaviour you should be able to re-mount a component and it should start working again from where you left off, pretty much, perhaps updating some signals' data if possible (see above).

emitOnce param default will be switched from false to true, or even removed (for streams like EventStream.fromValue). I'm thinking to maybe implement an evalOnStart(:=> myStream) function that returns an observable that re-creates and mirrors myStream every time it's started. That would help with a bunch of different streams like fromValue, periodic, fromFuture, etc.

Anything that has a resetOnStop param will have it removed or default to false (well I guess that's just EventStream.periodic()`

The main edge case here are async streams like ajax / delay / debounce. They will probably get their own treatment, but not sure what it is yet. With async logic it's really hard to come up with something that would work universally well, especially if you consider all the possible timing edge cases.

KeepAlive

One thing that annoys me about ownership lifecycles today it the inability to easily adjust them: #70

For example, say you make an ajax POST request using a stream, and you want to update your app's data store or cookies or something once the request finishes. But what if the user unmounts the component where this logic is defined while the request is still in flight? If the cookie-updating logic was bound inside that component, it will be deactivated once the component is unmounted, and will never happen. Alternatively, if you use unsafeWindowOwner for that logic, it might cause a memory leak if you're not careful to dispose of the subscription, which is not something you should be worrying about with Airstream.

So I'm thinking to provide some kind of keepAlive(maxEvents, maxSeconds) method that would keep the subscription running. I haven't figured out how to actually implement it yet in order for it to be the most useful, whether it should be on the observer side or the observable side, or a property of the Subscription class exposed conveniently via some helper in Laminar.

Have you run into this issue yourself?

Split operator will always return a Signal (not a stream)

You can still call split on streams, but it will return a signal whose initial value will be an empty collection. I went mad getting it to work properly, and now it does, but I don't think I can implement a properly semantic stream for this. It's really complicated. If you need a stream, just call .changes (Laminar doesn't care anymore so not sure why you'd need that). I guess Airstream could have done that for you but semantically the output of split is really a Signal as it relies heavily on its internal state.

Also, split will always provide a Signal as the third param in the render callback.

Observable completion

I don't think this will make it into v0.15.0, but while I have your attention, I will eventually also try to implement observable completion feature, see #23 #33 #1. Basically that means that once it's known that an observable won't ever emit any new values (not even after a restart), it proactively kills all of its subscriptions.

The interactions of observable completion feature with other features are really complicated. What I'd like from you is to tell me whether you want this feature, and if so, why. One argument I heard is that it will allow a concat method on streams – if that's the kind of feature that you want, please let me know how you plan to use it to help me visualize it better.

I have 2 questions/comments regarding the signals (using Airstream in the context of Laminar):

  1. A common usecase for me is:
child <-- someBoolean.signal.map{
    case false => emptyNode
    case true => div(.. /* some expensive component */ ..)
}

(perhaps with some laminext sugar or similar, but to show the idea)

Here I really only want to trigger when the signal changes, as it's inefficient to redraw for no reason, and any state in the big component is lost.

I'm not sure under what circumstances the signal would fire twice though? If I do someBoolean.set(true), can that result in multiple true events? If the price of this change is to modify this to someBoolean.signal.distinct.map{ that would be fine.

  1. Something that is perhaps not too related to this issue:

I often unintentionally start with something like:

child <-- bigCollection.map(_.size).signal.map{
     case 0 => emptyNode
     case _ => div(... /* some expensive component */ ..)
}

and then realise that this redraws for any change in bigCollection.size.. so I need to use bigCollection.map(_.size > 0).map{ in order to have a signal that only fires when relevant.

I guess this is the correct and expected behaviour though, and something I should just learn to keep in mind.

raquo commented

I think the new semantics for signals is a very good choice. It gives back control over == / distinct aspect which would be very useful for performance and/or complexity management. I found myself more then a few times in a limbo where I needed at the same time both a stream to handle the event semantics and signal to handle view aspect for the same data. In simple use cases you can go around this but in more complex scenarios (i.e. state mapping, multiple data sources combine, error handling, chains of async calls) it gets messy very quickly and sooner or later you come to the problem of "converting" streams to singals and vice-versa which is not sound at all.

raquo commented

Here are my thoughts:

  • No more == checks in signals:
    This will require a thorough review of my existing code but it will be worth it. The added control will simplify some places and require to be more explicit in others.
  • Signals will re-evaluate their current value after being re-started:
    I'm very excited for this, I've spent quite a bit of time hacking around this one.
    For instance, I have a text area which is resized automatically based on its content. Since it also needs to be hidden/displayed, the size must be computed again when it gets displayed. Simply using onMount doesn't work as the Signal feeding it is out of date.
  • Observables will stop resetting their internal state when they're stopped:
    Makes complete sense for the same reasons.
  • KeepAlive:
    I didn't run into a case where un-mounting the owner before getting a response was required
  • Split operator will always return a Signal (not a stream)
    I never felt the need for something else than a Signal from a split
  • Observable completion
    I didn't encounter a use case for this.
raquo commented

@phfroidmont Good feedback, thanks!

follow-up on rest of the points.

  • Signals will re-evaluate their current value after being re-started, Observables will stop resetting their internal state when they're stopped
    same experience as @phfroidmont changes are definitly worth it
  • KeepAlive:
    I had occassional issues that would be solved by this (async request cancelling and error handling with a "fast clicker" user)
  • Split operator will always return a Signal (not a stream)
    the only case I considred stream out of a split was to solve problems coming from == in Signal semantics which will be fixed.
  • Observable completion
    I had one use case where it would be useful. At the moment I use EventStream.merge which kind of works as one stream is just a single sync value. Other then that I did not encounter the need for EventStream.concat
  • No more == checks in signals:
    I think this makes sense. Additionally, we can add a distinctUntilChanged simlar to monix Observable to get back the old behavior
raquo commented

@ngbinh That's the plan, currently I have the following methods drafted:

  /** Distinct events (but keep all errors) by == (equals) comparison */
  def distinct: Self[A] = distinctBy(_ == _)

  /** Distinct events (but keep all errors) by reference equality (eq) */
  def distinctByRef(implicit ev: A <:< AnyRef): Self[A] = distinctBy(ev(_) eq ev(_))

  /** Distinct events (but keep all errors) by matching key
    * Note: `key(event)` might be evaluated more than once for each event
    */
  def distinctByKey(key: A => Any): Self[A] = distinctBy(key(_) == key(_))

  /** Distinct events (but keep all errors) using a comparison function
    *
    * @param fn (prev, next) => isSame
    */
  def distinctBy(fn: (A, A) => Boolean): Self[A] = distinctTry {
    case (Success(prev), Success(next)) => fn(prev, next)
    case _ => false
  }

  /** Distinct errors only (but keep all events) using a comparison function
    *
    * @param fn (prevErr, nextErr) => isSame
    */
  def distinctErrors(fn: (Throwable, Throwable) => Boolean): Self[A] = distinctTry {
    case (Failure(prevErr), Failure(nextErr)) => fn(prevErr, nextErr)
    case _ => false
  }

  /** Distinct all values (both events and errors) using a comparison function
    *
    * @param fn (prev, next) => isSame
    */
  def distinctTry(fn: (Try[A], Try[A]) => Boolean): Self[A]
raquo commented

Unfortunately KeepAlive mechanics won't make it into 0.15.0. I've looked into it, and it's very doable, but it's just too much work for this iteration. Current status as follows:

Done:

  • No more == checks in signals
  • Signals will re-evaluate their current value after being re-started
  • Observables will stop resetting their internal state when they're stopped
  • Split operator will always return a Signal (not a stream)
  • Take / drop / distinct operators
  • Lots of other small stuff

I'm currently wrapping up raquo/Laminar#95. Really happy with how that feature worked out. Now onto "the second 90%"...

TODO for v0.15.0:

  • Fix ergonomics issues with the new sync-on-restart logic
  • Finish raquo/Laminar#95 and the corresponding revamp of SDT styles API (shaping up nicely so far)
  • Other, minor Laminar things like composeEventsFlat
  • If I can implement splitByIndex in a couple hours, I'll do that
  • Extract my scala-overhead-free JsMap / JsArray interfaces into a separate library
  • Rename all event stream classes – drop "event" from the names
  • Upgrade my own Laminar app to unreleased versions to make sure the migration is relatively smooth
  • Update documentation and website examples
  • Write blog post / migration guide
  • Release an RC and collect feedback

So, the hard parts are done, but quite a lot of work still ahead. All of that will probably take me a few weeks.

That sounds like a good change to me. I just got bit by the Signal deduplicating events issue, because I didn't pay sufficient attention to the return type of 'startWith' and its implications.

raquo commented

I've implemented most things here in Airstream 15. Did not have time for observable completion and keepalive yet.