New: Rework Ownership and Lifecycle events

- Use new DynamicOwner from Airstream
- Element subscriptions are only activated on element mount, not on element init
- This fixes the main memory management gotcha: Fixes #33
- New pilot subscription fixes #47

CHANGELOG.md

@@ -4,6 +4,12 @@ Breaking changes in **bold**.
 
 #### v0.8 – TBD
 
+* **API: Lifecycle events & Ownership overhaul**
+  * <TODO: Elaborate> Use Airstream's new `DynamicOwner` and `DynamicSubscription` features: `ReactiveElement` subscriptions are now activated only when the element is mounted, not when it is created.
+  * <TODO: Elaborate> Transaction delay for `mountEvents` now applies to all element subscriptions
+  * <TODO: Elaborate> Allow remounting of unmounted components (document pilot subscription memory management gotcha) 
+  * `NodeWasDiscarded` event is not fired anymore. See `NodeWillUnmount`.
+* **API: Hide `parentChangeEvents` and `maybeParentSignal`. This functionality is not available anymore as it requires undesirable tradeoffs.
 * **API: Rename types:** `ReactiveHtmlBuilders` -> `HtmlBuilders`, `ReactiveSvgBuilders` -> `SvgBuilders`, `ReactiveRoot` -> `RootNode`, `ReactiveComment` -> `CommentNode`, `ReactiveText` -> `TextNode`, `ReactiveChildNode` -> `ChildNode`
 * **API: Move `ChildrenCommand` out of the poorly named `collection` package**
 * **API: Eliminate dependency on _Scala DOM Builder_**

build.sbt

@@ -6,7 +6,7 @@ enablePlugins(ScalaJSBundlerPlugin)
 // resolvers += Resolver.sonatypeRepo("snapshots")
 
 libraryDependencies ++= Seq(
-  "com.raquo" %%% "airstream" % "0.7.2",
+  "com.raquo" %%% "airstream" % "0.7.3-SNAPSHOT",
   "com.raquo" %%% "domtypes" % "0.9.6",
   "com.raquo" %%% "domtestutils" % "0.9.1" % Test,
   "org.scalatest" %%% "scalatest" % "3.0.8" % Test

docs/Documentation.md

@@ -164,7 +164,7 @@ Thanks to the `optionToModifier` implicit mentioned above, you can use `Option[R
 
 And so, an empty DOM node can be created with... `emptyNode`.
 
-// @NC What type actually
+// @nc What type actually
 
 ```scala
 val node: Node = if (foo) element else emptyNode

src/main/scala/com/raquo/laminar/api/Airstream.scala

@@ -16,8 +16,6 @@ trait Airstream {
 
   type Observer[-A] = airstream.core.Observer[A]
 
-  type Owned = airstream.ownership.Owned
-
   type Owner = airstream.ownership.Owner
 
   type Ref[+A <: AnyRef] = airstream.util.Ref[A]
@@ -28,7 +26,7 @@ trait Airstream {
 
   type StrictSignal[+A] = airstream.signal.StrictSignal[A]
 
-  type Subscription = airstream.core.Subscription
+  type Subscription = airstream.ownership.Subscription
 
   type Val[A] = airstream.signal.Val[A]
 

src/main/scala/com/raquo/laminar/api/Laminar.scala

@@ -160,7 +160,7 @@ private[laminar] object Laminar
 
   @inline def render(
     container: dom.Element,
-    rootNode: nodes.ChildNode[dom.Element]
+    rootNode: nodes.ReactiveElement.Base
   ): RootNode = {
     new RootNode(container, rootNode)
   }

src/main/scala/com/raquo/laminar/lifecycle/MountEvent.scala

@@ -4,13 +4,20 @@ package com.raquo.laminar.lifecycle
 /** Mount Events can inform you about certain stages of the Node's lifecycle,
   * letting you perform the required initialization / cleanup.
   *
-  * They are also used internally for managing the lifecycle of stream subscriptions.
+  * They are also used internally for managing the lifecycle of element subscriptions.
   *
   * A Node is considered mounted if it's present in the DOM, i.e. it has `dom.document`
   * among its ancestors. A node that is not mounted is considered unmounted.
   *
   * All MountEvent-s are cascading: if a certain event is fired on a node, it will also
   * fire on all descendants of that node.
+  *
+  * Important: All MountEvent-s are emitted AFTER the current Airstream transaction completes.
+  * That means that any code that listens to those events including all element subscriptions
+  * will be run AFTER whatever event caused the element to get mounted has finished propagating.
+  * So don't expect to fire an event, mount an element in response to that event, and have one
+  * of this new element's subscriptions also react to the same event. The subscription will not see
+  * that event. See the ignored test in WeirdCasesSpec.
   */
 sealed trait MountEvent
 
@@ -22,30 +29,11 @@ sealed trait MountEvent
 object NodeDidMount extends MountEvent
 
 /** `NodeWillUnmount` event fires when a previously mounted node is about to be unmounted.
-  * Note: when the even fires the node is still mounted.
+  * Note: when the event fires, the node is still mounted.
   *
   * Internally, Laminar will deactivate the node's subscriptions when it's unmounted,
-  * except for the internal subscription that listens to mountEvents (so that we can
+  * except for the internal pilot subscription that listens to mountEvents (so that we can
   * re-activate the other subscriptions when the node gets mounted again).
-  *
-  * Note: currently, every `NodeWillUnmount` event is followed by a `NodeWillBeDiscarded`
-  * event which deactivates the subscription that listens to mountEvents
   */
 object NodeWillUnmount extends MountEvent
 
-// @TODO Provide a way to avoid discarding nodes (and how do we manually discard them then? Discard when parent is discarded...? Think about it and update the docs before implementing)
-// @TODO Think about the inheritance mechanics of discarding, and document all this
-/** `NodeWasDiscarded` event fires after `NodeWillUnmount` unless the end
-  * user specified that the node should not be discarded when it's unmounted.
-  *
-  * A discarded node is defined as a node that is unmounted and will not be mounted again.
-  * The latter is a promise by the end user, not enforced by Laminar.
-  *
-  * Internally, when discarding a node Laminar kills all of the subscriptions that it owned.
-  * NodeWasDiscarded event is your last chance to do any cleanup related to this node.
-  *
-  * Note: There currently is no way for the end user to specify that the node should not be
-  * discarded, so the end user implicitly promises to not re-mount nodes which have
-  * been unmounted.
-  */
-object NodeWasDiscarded extends MountEvent

src/main/scala/com/raquo/laminar/nodes/ChildNode.scala

@@ -36,7 +36,7 @@ trait ChildNode[+Ref <: dom.Node]
     *
     * @param maybeNextParent  `None` means this node is about to be detached form its parent
     */
-  @inline def willSetParent(maybeNextParent: Option[ParentNode.Base]): Unit = {}
+  @inline def willSetParent(maybeNextParent: Option[ParentNode.Base]): Unit = ()
 
   override def apply(parentNode: ParentNode.Base): Unit = {
     // @TODO[Performance] Consider making ChildNode -> Modifier conversion implicit instead (but watch compile times)

src/main/scala/com/raquo/laminar/nodes/ReactiveElement.scala

@@ -1,15 +1,15 @@
 package com.raquo.laminar.nodes
 
-import com.raquo.airstream.core.{Observable, Observer, Subscription}
-import com.raquo.airstream.eventbus.{EventBus, EventBusSource, WriteBus}
+import com.raquo.airstream.core.{Observable, Observer}
+import com.raquo.airstream.eventbus.{EventBus, WriteBus}
 import com.raquo.airstream.eventstream.EventStream
-import com.raquo.airstream.ownership.{Owned, Owner}
+import com.raquo.airstream.ownership.{DynamicOwner, DynamicSubscription, Owner, Subscription}
 import com.raquo.airstream.signal.Signal
 import com.raquo.domtypes
 import com.raquo.domtypes.generic.keys.EventProp
 import com.raquo.laminar.DomApi
 import com.raquo.laminar.emitter.EventPropEmitter
-import com.raquo.laminar.lifecycle.{MountEvent, NodeDidMount, NodeWasDiscarded, NodeWillUnmount, ParentChangeEvent}
+import com.raquo.laminar.lifecycle.{MountEvent, NodeDidMount, NodeWillUnmount, ParentChangeEvent}
 import com.raquo.laminar.nodes.ChildNode.isParentMounted
 import com.raquo.laminar.nodes.ReactiveElement.noMountEvents
 import com.raquo.laminar.receivers.{ChildReceiver, ChildrenCommandReceiver, ChildrenReceiver, MaybeChildReceiver, TextChildReceiver}
@@ -21,43 +21,43 @@ import scala.collection.mutable
 trait ReactiveElement[+Ref <: dom.Element]
   extends ChildNode[Ref]
   with ParentNode[Ref]
-  with domtypes.generic.nodes.Element
-  with Owner {
+  with domtypes.generic.nodes.Element {
 
   // @TODO[Naming] We reuse EventPropSetter to represent an active event listener. Makes for a bit confusing naming.
   private[ReactiveElement] var _maybeEventListeners: Option[mutable.Buffer[EventPropSetter[_]]] = None
 
   @inline def maybeEventListeners: Option[List[EventPropSetter[_]]] = _maybeEventListeners.map(_.toList)
 
+  private[this] val dynamicOwner: DynamicOwner = new DynamicOwner
+
   /** Event bus that emits parent change events.
     * For efficiency, it is only populated when someone accesses [[parentChangeEvents]]
     */
-  private[laminar] var maybeParentChangeBus: Option[EventBus[ParentChangeEvent]] = None
+  private[this] var maybeParentChangeBus: Option[EventBus[ParentChangeEvent]] = None
 
   /** Event bus that emits this node's mount events.
     * For efficiency, it is only populated when someone accesses [[thisNodeMountEvents]]
     */
-  private[laminar] var maybeThisNodeMountEventBus: Option[EventBus[MountEvent]] = None
+  private[this] var maybeThisNodeMountEventBus: Option[EventBus[MountEvent]] = None
 
   /** Stream of parent change events for this node.
-    * For efficiency, it is lazy loaded, only being initialized when accessed,
-    * either directly or (more commonly) as a dependency of [[mountEvents]]
+    * For efficiency, it is lazy loaded, only being initialized when accessed
     */
-  lazy val parentChangeEvents: EventStream[ParentChangeEvent] = {
+  private[this] lazy val parentChangeEvents: EventStream[ParentChangeEvent] = {
     val parentChangeBus = new EventBus[ParentChangeEvent]
     maybeParentChangeBus = Some(parentChangeBus)
     parentChangeBus.events
   }
 
-  lazy val maybeParentSignal: Signal[Option[ParentNode.Base]] = parentChangeEvents
+  private[this] lazy val maybeParentSignal: Signal[Option[ParentNode.Base]] = parentChangeEvents
     .filter(_.alreadyChanged) // @TODO[Integrity] is this important?
     .map(_.maybeNextParent)
     .toSignal(maybeParent)
 
   /** Emits mount events that were caused by this element's parent changing its parent,
     * or any such changes up the chain. Does not include mount events triggered by this
     * element changing its parent - see [[thisNodeMountEvents]] for that. */
-  private lazy val ancestorMountEvents: EventStream[MountEvent] = {
+  private[this] lazy val ancestorMountEvents: EventStream[MountEvent] = {
     maybeParentSignal.map {
       case Some(nextParent: ReactiveElement[_]) =>
         nextParent.mountEvents
@@ -69,7 +69,7 @@ trait ReactiveElement[+Ref <: dom.Element]
   /** Emits mount events caused by this node changing its parent. Does not include mount
     * events triggered by changes higher in the hierarchy (grandparent and up) – see
     * [[ancestorMountEvents]] for that. */
-  private lazy val thisNodeMountEvents: EventStream[MountEvent] = {
+  private[this] lazy val thisNodeMountEvents: EventStream[MountEvent] = {
     val thisNodeMountEventBus = new EventBus[MountEvent]
     maybeThisNodeMountEventBus = Some(thisNodeMountEventBus)
     thisNodeMountEventBus.events
@@ -80,6 +80,25 @@ trait ReactiveElement[+Ref <: dom.Element]
     EventStream.merge(ancestorMountEvents, thisNodeMountEvents)
   }
 
+  // @nc normally this would not work memory management wise, as we create a sub on element init, and never even bother to kill it
+  //  - however, this sub only looks at mount events
+  //  - thisNodeMountEvents are contained within this node and don't require any leaky resources
+  //  - ancestorMountEvents look at parent nodes' mountEvents
+  //    - while this node is mounted, all ancestors are also mounted and that is ok
+  //    - if this node becomes unmounted, the ancestor chain includes only other unmounted nodes
+  //    - so this will lock together that ancestor chain, but that's like meh
+  //    - even if we re-mounted this node to different parents / ancestors, this chain would be broken as `mountEvents` would now point elsewhere
+  //    - as long as we understand and document this limitation, I think it's ok
+  // @nc Can we initialize this conditionally, only for elements that need it?
+  {
+    val pilotSubscriptionOwner: Owner = new Owner {}
+
+    mountEvents.foreach{
+      case NodeDidMount => dynamicOwner.activate()
+      case NodeWillUnmount => dynamicOwner.deactivate()
+    }(pilotSubscriptionOwner)
+  }
+
   /** Create and get a stream of events on this node */
   def events[Ev <: dom.Event](
     eventProp: EventProp[Ev],
@@ -108,45 +127,42 @@ trait ReactiveElement[+Ref <: dom.Element]
 
   final def <--[V](childrenCommandReceiver: ChildrenCommandReceiver.type): ChildrenCommandReceiver = new ChildrenCommandReceiver(this)
 
-  // @TODO[Naming] Not a fan of `subscribeO` name, but it needs to be different from `subscribe` for type inference to work
+  // @TODO[Naming] Not a fan of `subscribeS` / `subscribeO` names, but it needs to be different from `subscribe` for type inference to work
+  // @TODO[API] Consider having subscribe() return a Subscribe object that has several apply methods on it to reign in this madness
+  //  - Also, do we really need currying here?
+
+  def subscribeS(getSubscription: Owner => Subscription): DynamicSubscription = {
+    new DynamicSubscription(dynamicOwner, getSubscription)
+  }
 
-  def subscribeO[V](observable: Observable[V])(observer: Observer[V]): Subscription = {
-    observable.addObserver(observer)(owner = this)
+  def subscribeO[V](observable: Observable[V])(observer: Observer[V]): DynamicSubscription = {
+    subscribeS(owner => observable.addObserver(observer)(owner))
   }
 
   // @TODO[Naming] Rename to subscribeThisO?
-  def subscribeO[V](getObservable: this.type => Observable[V])(observer: Observer[V]): Subscription = {
+  def subscribeO[V](getObservable: this.type => Observable[V])(observer: Observer[V]): DynamicSubscription = {
     subscribeO(getObservable(this))(observer)
   }
 
-  def subscribe[V](observable: Observable[V])(onNext: V => Unit): Subscription = {
+  def subscribe[V](observable: Observable[V])(onNext: V => Unit): DynamicSubscription = {
     subscribeO(observable)(Observer(onNext))
   }
 
   // @TODO[Naming] Rename to subscribeThis?
-  def subscribe[V](getObservable: this.type => Observable[V])(onNext: V => Unit): Subscription = {
+  def subscribe[V](getObservable: this.type => Observable[V])(onNext: V => Unit): DynamicSubscription = {
     subscribeO(getObservable(this))(Observer(onNext))
   }
 
   def subscribeBus[V](
     sourceStream: EventStream[V],
     targetBus: WriteBus[V]
-  ): EventBusSource[V] = {
-    targetBus.addSource(sourceStream)(owner = this)
-  }
-
-  /** Check whether the node is currently mounted.
-    *
-    * You can use this method to simplify your code and possibly improve performance
-    * where you'd otherwise need to subscribe to and transform [[mountEvents]]
-    */
-  def isMounted: Boolean = {
-    isParentMounted(maybeParent)
+  ): DynamicSubscription = {
+    subscribeS(owner => targetBus.addSource(sourceStream)(owner))
   }
 
   /** This hook is exposed by Scala DOM Builder for this exact purpose */
   override def willSetParent(maybeNextParent: Option[ParentNode.Base]): Unit = {
-    // @TODO[Integrity] In dombuilder, willSetParent is called at incorrect time in ParentNode.appendChild
+    // super.willSetParent(maybeNextParent) // default implementation is a noop, so no need to call it.
     // dom.console.log(">>>> willSetParent", this.ref.tagName + "(" + this.ref.textContent + ")", maybeParent == maybeNextParent, maybeParent.toString, maybeNextParent.toString)
     if (maybeNextParent != maybeParent) {
       maybeParentChangeBus.foreach(bus => {
@@ -171,37 +187,21 @@ trait ReactiveElement[+Ref <: dom.Element]
     super.setParent(maybeNextParent)
     // dom.console.log(">>>> setParent", this.ref.tagName + "(" + this.ref.textContent + ")", maybePrevParent == maybeNextParent, maybePrevParent.toString, maybeNextParent.toString)
     if (maybeNextParent != maybePrevParent) {
-      maybeParentChangeBus.foreach( bus => {
-        bus.writer.onNext(ParentChangeEvent(
+      maybeParentChangeBus.foreach(bus => {
+        val ev = ParentChangeEvent(
           alreadyChanged = true,
           maybePrevParent = maybePrevParent,
           maybeNextParent = maybeNextParent
-        ))
+        )
+        bus.writer.onNext(ev)
       })
       val prevParentIsMounted = isParentMounted(maybePrevParent)
       val nextParentIsMounted = isParentMounted(maybeNextParent)
       if (!prevParentIsMounted && nextParentIsMounted) {
         maybeThisNodeMountEventBus.foreach(_.writer.onNext(NodeDidMount))
       }
-      if (prevParentIsMounted && !nextParentIsMounted) {
-        maybeThisNodeMountEventBus.foreach { bus =>
-          bus.writer.onNext(NodeWasDiscarded) // @TODO this should be optional
-        }
-      }
     }
   }
-
-  override protected[this] def onOwned(owned: Owned): Unit = {
-    val isFirstSubscription = possessions.length == 1
-    if (isFirstSubscription) {
-      // Create a subscription that will kill all subscriptions that this element owns when it is discarded.
-      // Note: Once this subscription is created, it will persist until this element is discarded.
-      //       This might be suboptimal in a narrow range of conditions, I don't think anyone will run into those.
-      // @TODO[Performance] ^^^^ The above can be addressed by e.g. overriding onKilledExternally (but permissions)
-      mountEvents.filter(_ == NodeWasDiscarded).foreach(_ => killPossessions() )(owner = this)
-    }
-  }
-
 }
 
 object ReactiveElement {

src/main/scala/com/raquo/laminar/nodes/ReactiveHtmlElement.scala

@@ -20,7 +20,8 @@ class ReactiveHtmlElement[+Ref <: dom.html.Element](val tag: HtmlTag[Ref])
   final def <--[V](focus: FocusReceiver.type): FocusReceiver = new FocusReceiver(this)
 
   override def toString: String = {
-    s"ReactiveHtmlElement(${ref.outerHTML})"
+    // `ref` is not available inside ReactiveElement's constructor due to initialization order, so fall back to `tag`.
+    s"ReactiveHtmlElement(${ if (ref != null) ref.outerHTML else s"tag=${tag.name}"})"
   }
 }
 

src/main/scala/com/raquo/laminar/nodes/ReactiveSvgElement.scala

@@ -14,7 +14,8 @@ class ReactiveSvgElement[+Ref <: dom.svg.Element](val tag: SvgTag[Ref])
   final def <--[V](attr: ReactiveSvgAttr[V]): SvgAttrReceiver[V] = new SvgAttrReceiver(attr, this)
 
   override def toString: String = {
-    s"ReactiveSvgElement(${ref.outerHTML})"
+    // `ref` is not available inside ReactiveElement's constructor due to initialization order, so fall back to `tag`.
+    s"ReactiveSvgElement${ if (ref != null) ref.outerHTML else s"tag=${tag.name}"})"
   }
 }
 

src/main/scala/com/raquo/laminar/nodes/RootNode.scala

@@ -4,9 +4,14 @@ import org.scalajs.dom
 
 class RootNode(
   val container: dom.Element,
-  val child: ChildNode[dom.Element]
+  val child: ReactiveElement.Base
 ) extends ParentNode[dom.Element] {
 
+  // @nc Do we actually need this? If so, document the exact reason
+  if (!ChildNode.isNodeMounted(container)) {
+    throw new Exception("Unable to mount Laminar RootNode into an unmounted container.")
+  }
+
   /** When we create a Root, we don't want to create a new HTML Element, we want to
     * use a reference to an existing element, the container.
     */