rx-html (fix): Properly cancel DOM elements when Rx.filter condition …

…changes (#3045)
wvlet · Jul 5, 2023 · 57d00ab · 57d00ab
1 parent 2b7ffc6
commit 57d00ab
Show file tree

Hide file tree

Showing 10 changed files with 108 additions and 30 deletions.
diff --git a/airframe-rx-html/.js/src/main/scala/wvlet/airframe/rx/html/DOMRenderer.scala b/airframe-rx-html/.js/src/main/scala/wvlet/airframe/rx/html/DOMRenderer.scala
@@ -13,7 +13,7 @@
  */
 package wvlet.airframe.rx.html
 import org.scalajs.dom
-import wvlet.airframe.rx.{Cancelable, Rx, RxOps}
+import wvlet.airframe.rx.{Cancelable, OnNext, Rx, RxOps, RxRunner}
 import wvlet.log.LogSupport
 
 import scala.scalajs.js
@@ -142,12 +142,17 @@ object DOMRenderer extends LogSupport {
         case rx: RxOps[_] =>
           val (start, end) = node.createMountSection()
           var c1           = Cancelable.empty
-          val c2 = rx.subscribe { value =>
+          val c2 = RxRunner.runContinuously(rx) { ev =>
             // Remove the previous binding from the DOM
             node.clearMountSection(start, end)
             // Cancel the previous binding
             c1.cancel
-            c1 = traverse(value, Some(start))
+            ev match {
+              case OnNext(value) =>
+                c1 = traverse(value, Some(start))
+              case other =>
+                c1 = Cancelable.empty
+            }
           }
           Cancelable.merge(c1, c2)
         case a: HtmlAttribute =>
@@ -215,10 +220,10 @@ object DOMRenderer extends LogSupport {
         case Some(x) =>
           traverse(x, anchor)
         case s: Iterable[_] =>
-          val cancellables = for (el <- s) yield {
+          val cancelables = for (el <- s) yield {
             traverse(el, anchor)
           }
-          Cancelable.merge(cancellables)
+          Cancelable.merge(cancelables)
         case other =>
           throw new IllegalArgumentException(s"unsupported class ${other}")
       }
@@ -274,15 +279,17 @@ object DOMRenderer extends LogSupport {
           traverse(x)
         case rx: RxOps[_] =>
           var c1 = Cancelable.empty
-          val c2 = rx.run { value =>
+          val c2 = RxRunner.runContinuously(rx) { ev =>
             // Cancel the previous binding
             c1.cancel
-            c1 = traverse(value)
+            ev match {
+              case OnNext(value) =>
+                c1 = traverse(value)
+              case other =>
+                c1 = Cancelable.empty
+            }
           }
           Cancelable.merge(c1, c2)
-        // TODO: Add RxElement rendering
-        // case r: RxElement =>
-        //
         case f: Function0[_] =>
           node.setEventListener(a.name, { (_: dom.Event) => f() })
         case f: Function1[dom.Node @unchecked, _] =>
@@ -301,7 +308,7 @@ object DOMRenderer extends LogSupport {
                 htmlNode.style.cssText = value
               }
               Cancelable { () =>
-                if (htmlNode != null && value.nonEmpty) {
+                if (htmlNode != null && a.append && value.nonEmpty) {
                   val newAttributeValue = removeStyleValue(htmlNode.style.cssText, value)
                   htmlNode.style.cssText = newAttributeValue
                 }
@@ -332,9 +339,8 @@ object DOMRenderer extends LogSupport {
                 value
               }
               setAttribute(newAttrValue)
-
               Cancelable { () =>
-                if (htmlNode != null && htmlNode.hasAttribute(a.name)) {
+                if (htmlNode != null && a.append && htmlNode.hasAttribute(a.name)) {
                   val v = htmlNode.getAttribute(a.name)
                   if (v != null) {
                     // remove the appended value

diff --git a/airframe-rx-html/.js/src/test/scala/wvlet/airframe/http/rx/html/RxRenderingTest.scala b/airframe-rx-html/.js/src/test/scala/wvlet/airframe/http/rx/html/RxRenderingTest.scala
@@ -14,7 +14,7 @@
 package wvlet.airframe.http.rx.html
 
 import org.scalajs.dom.{HTMLElement, document}
-import wvlet.airframe.rx.{Cancelable, Rx}
+import wvlet.airframe.rx.{Cancelable, Rx, html}
 import wvlet.airframe.rx.html.{DOMRenderer, Embedded, RxElement}
 import wvlet.airspec.AirSpec
 import wvlet.airframe.rx.html._
@@ -306,13 +306,32 @@ object RxRenderingTest extends AirSpec {
     updated shouldBe true
   }
 
+  test("refresh attribute with RxVar") {
+    val show = Rx.variable(true)
+    val e = new RxElement {
+      override def render: RxElement = {
+        div(
+          show.when(_ == true).map(_ => cls += "active")
+        )
+      }
+    }
+    val (n, c) = render(e)
+    n.outerHTML shouldBe """<div class="active"></div>"""
+    show := false
+    n.outerHTML shouldBe """<div></div>"""
+    show := true
+    n.outerHTML shouldBe """<div class="active"></div>"""
+  }
+
   test("append cls attribute") {
     val selected = Rx.variable("home")
     val e = new RxElement {
       override def render: RxElement = {
         div(
           cls -> "item",
-          selected.map(x => (cls += "active").when(x == "home")),
+          selected.when(_ == "home").map { x =>
+            cls += "active"
+          },
           cls += "text-primary",
           selected
         )
@@ -332,7 +351,7 @@ object RxRenderingTest extends AirSpec {
       override def render: RxElement = {
         div(
           style -> "color: white;",
-          selected.map(x => (style += "font-size: 10px;").when(x == "home")),
+          selected.when(_ == "home").map(_ => style += "font-size: 10px;"),
           selected
         )
       }
@@ -350,7 +369,7 @@ object RxRenderingTest extends AirSpec {
     val e = new RxElement {
       override def render: RxElement = {
         div(
-          selected.map(x => (cls += "active").when(x == "home")),
+          selected.when(_ == "home").map(x => cls += "active"),
           selected
         )
       }

diff --git a/airframe-rx-html/src/main/scala/wvlet/airframe/rx/html/HtmlNode.scala b/airframe-rx-html/src/main/scala/wvlet/airframe/rx/html/HtmlNode.scala
@@ -15,10 +15,12 @@ package wvlet.airframe.rx.html
 import wvlet.airframe.rx.html.RxEmbedding._
 
 trait HtmlNode extends HtmlNodeBase {
+  @deprecated("Use html.when(cond, node)", since = "23.7.0")
   def when(cond: => Boolean): HtmlNode = {
     if (cond) this else HtmlNode.empty
   }
 
+  @deprecated("Use html.when(!cond, node)", since = "23.7.0")
   def unless(cond: => Boolean): HtmlNode = {
     if (cond) HtmlNode.empty else this
   }

diff --git a/airframe-rx-html/src/main/scala/wvlet/airframe/rx/html/TagsExtra.scala b/airframe-rx-html/src/main/scala/wvlet/airframe/rx/html/TagsExtra.scala
@@ -62,6 +62,7 @@ trait TagsExtra {
   def head: HtmlElement = tag("head")
   // Defines a header for a document or section
   def header: HtmlElement = tag("header")
+
   // Defines a thematic change in the content
 
   // Defines marked/highlighted text

diff --git a/airframe-rx-html/src/main/scala/wvlet/airframe/rx/html/package.scala b/airframe-rx-html/src/main/scala/wvlet/airframe/rx/html/package.scala
@@ -13,7 +13,6 @@
  */
 package wvlet.airframe.rx
 
-import wvlet.airframe.rx.html.{HtmlAttributeOf, HtmlElement, HtmlEventHandlerOf, Namespace}
 import wvlet.log.LogSupport
 
 /**
@@ -43,6 +42,22 @@ package object html extends HtmlCompat with RxEmbedding {
 
   private[rx] case class RxCode(rxElements: Seq[RxElement], sourceCode: String)
 
+  /**
+    * Render an element only when the condition is true
+    *
+    * @param cond
+    * @param body
+    * @return
+    *   rendered element or empty
+    */
+  def when(cond: Boolean, body: => HtmlNode): HtmlNode = {
+    if (cond) {
+      body
+    } else {
+      HtmlNode.empty
+    }
+  }
+
   /**
     * Holder for embedding various types as tag contents
     *

diff --git a/airframe-rx-html/src/test/scala/wvlet/airframe/rx/html/HtmlTest.scala b/airframe-rx-html/src/test/scala/wvlet/airframe/rx/html/HtmlTest.scala
@@ -14,6 +14,7 @@
 package wvlet.airframe.rx.html
 
 import wvlet.airframe.rx.Rx
+import wvlet.airframe.rx.html._
 import wvlet.airframe.rx.html.all._
 import wvlet.airframe.rx.html.svgTags._
 import wvlet.airspec.AirSpec
@@ -39,10 +40,10 @@ class HtmlTest extends AirSpec {
       None,
       Rx.variable(1),
       Iterable("a", "b", "c"),
-      (cls -> "main").when(1 == 1),
-      (cls -> "main2").unless(1 == 1),
-      (cls -> "main").when(1 != 1),
-      (cls -> "main2").unless(1 != 1)
+      when(1 == 1, cls -> "main"),
+      when(1 != 1, cls -> "main2"),
+      when(1 != 1, cls -> "main"),
+      when(1 == 1, cls -> "main2")
     )
   }
 

diff --git a/airframe-rx/src/main/scala/wvlet/airframe/rx/Cancelable.scala b/airframe-rx/src/main/scala/wvlet/airframe/rx/Cancelable.scala
@@ -13,15 +13,18 @@
  */
 package wvlet.airframe.rx
 
-import scala.util.Try
+import wvlet.log.LogSupport
+
+import scala.util.{Failure, Try}
+import scala.util.control.NonFatal
 
 /**
   */
 trait Cancelable {
   def cancel: Unit = {}
 }
 
-object Cancelable {
+object Cancelable extends LogSupport {
   val empty: Cancelable = new Cancelable {}
 
   def apply(canceller: () => Unit): Cancelable =
@@ -39,15 +42,35 @@ object Cancelable {
     }
   }
 
-  def merge(lst: Iterable[Cancelable]): Cancelable = {
+  def merge(it: Iterable[Cancelable]): Cancelable = {
+    val lst = it.toIndexedSeq
     lst.size match {
       case 1 => lst.head
       case _ =>
-        val nonEmpty = lst.filter(_ != Cancelable.empty).toIndexedSeq
+        val nonEmpty = lst.filter(_ != Cancelable.empty)
         if (nonEmpty.isEmpty) {
           Cancelable.empty
         } else {
-          Cancelable { () => nonEmpty.map { c => Try(c.cancel) } }
+          Cancelable { () =>
+            val failures =
+              nonEmpty
+                .map(c => Try(c.cancel))
+                .collect { case Failure(ex) =>
+                  warn(ex)
+                  ex
+                }
+            failures.size match {
+              case 0 => // ok
+              case 1 =>
+                throw failures.head
+              case n if n > 1 =>
+                warn(s"Multiple exception occurred while cancelling")
+                for (f <- failures.tail) {
+                  warn(f)
+                }
+                throw failures.head
+            }
+          }
         }
     }
   }

diff --git a/airframe-rx/src/main/scala/wvlet/airframe/rx/Rx.scala b/airframe-rx/src/main/scala/wvlet/airframe/rx/Rx.scala
@@ -98,10 +98,18 @@ trait Rx[+A] extends RxOps[A] {
   def withName(name: String): Rx[A] = NamedOp(this, name)
 
   // def map[B](f: A => B): Rx[B]           = MapOp[A, B](this, f)
-  def flatMap[B](f: A => Rx[B]): Rx[B]   = FlatMapOp(this, f)
-  def filter(f: A => Boolean): Rx[A]     = FilterOp(this, f)
+  def flatMap[B](f: A => Rx[B]): Rx[B] = FlatMapOp(this, f)
+  def filter(f: A => Boolean): Rx[A]   = FilterOp(this, f)
+
   def withFilter(f: A => Boolean): Rx[A] = FilterOp(this, f)
 
+  /**
+    * An alias of filter
+    * @param conf
+    * @return
+    */
+  def when(cond: A => Boolean): Rx[A] = FilterOp(this, cond)
+
   /**
     * Combine two Rx streams to form a sequence of pairs. This will emit a new pair when both of the streams are
     * updated.

diff --git a/airframe-rx/src/main/scala/wvlet/airframe/rx/RxRunner.scala b/airframe-rx/src/main/scala/wvlet/airframe/rx/RxRunner.scala
@@ -139,7 +139,8 @@ class RxRunner(
                 case Success(true) =>
                   effect(OnNext(x))
                 case Success(false) =>
-                  // Skip unmatched element
+                  // Notify the completion of the stream, but continue subscription to the upstream
+                  effect(OnCompletion)
                   RxResult.Continue
                 case Failure(e) =>
                   effect(OnError(e))

diff --git a/airframe-rx/src/test/scala/wvlet/airframe/rx/RxTest.scala b/airframe-rx/src/test/scala/wvlet/airframe/rx/RxTest.scala
@@ -651,6 +651,7 @@ object RxTest extends AirSpec {
     test("filter") {
       eval(Rx.sequence(1, 2, 3).filter(_ % 2 == 1)) shouldBe Seq(
         OnNext(1),
+        OnCompletion,
         OnNext(3),
         OnCompletion
       )
@@ -758,6 +759,7 @@ object RxTest extends AirSpec {
         OnNext(1),
         OnError(ex),
         OnNext(3),
+        OnCompletion,
         OnNext(5)
       )
     }