Fix: Bug in JsPriorityQueue implementation that produced glitches in …

…complex cases.

Specifically, when two or more observables were pending in the same transaction at the same time, if one of them actually synchronously depended on the other, it could fire ahead of the observable that it depended on, causing a glitch. Whether this actually happened also depended on the order of (internal) observers in the observable graph, you could avoid this bug just by luck. This bug could affect nested combineWith or delaySync observables, resulting in extraneous events (glitches) in case of combineWith, or observables firing in the wrong order (in case of delaySync).

See https://gitter.im/Laminar_/Lobby?at=6007655b36db01248a8bf5a9 for the original bug report.

src/main/scala/com/raquo/airstream/util/JsPriorityQueue.scala

@@ -2,23 +2,22 @@ package com.raquo.airstream.util
 
 import scala.scalajs.js
 
-// @TODO[Test] add tests for this class
-
 class JsPriorityQueue[A](getRank: A => Int) {
 
   private[this] val queue: js.Array[A] = js.Array()
 
   def enqueue(item: A): Unit = {
+    val itemRank = getRank(item)
     var insertAtIndex = 0
     var foundHigherRank = false
     while (
-      insertAtIndex < queue.length &&
-        !foundHigherRank
+      insertAtIndex < queue.length && !foundHigherRank
     ) {
-      if (getRank(queue(insertAtIndex)) <= getRank(item)) {
+      if (getRank(queue(insertAtIndex)) > itemRank) {
         foundHigherRank = true
+      } else {
+        insertAtIndex += 1
       }
-      insertAtIndex += 1
     }
     queue.splice(index = insertAtIndex, deleteCount = 0, item) // insert at index
   }
@@ -40,4 +39,6 @@ class JsPriorityQueue[A](getRank: A => Int) {
   @inline def isEmpty: Boolean = size == 0
 
   @inline def nonEmpty: Boolean = !isEmpty
+
+  def debugQueue: List[A] = queue.toList
 }

src/test/scala/com/raquo/airstream/timing/SyncDelayEventStreamSpec.scala

@@ -99,4 +99,100 @@ class SyncDelayEventStreamSpec extends UnitSpec {
     calculations.clear()
     effects.clear()
   }
+
+  it("multi-level, nested, dependent sync observables") {
+
+    // See https://gitter.im/Laminar_/Lobby?at=6007655b36db01248a8bf5a9 and below
+
+    implicit val testOwner: TestableOwner = new TestableOwner
+
+    val v = Var(0)
+
+    val calculations = mutable.Buffer[Calculation[Int]]()
+
+    val signal1 = v.signal
+      .map(Calculation.log("signal1", calculations))
+
+    val signal2 = signal1
+      .map(ev => ev)
+      .map(Calculation.log("signal2", calculations))
+
+    val signal3 = signal2
+      .composeChanges { signal2Changes =>
+        signal2Changes
+          .delaySync(signal1.changes)
+          .map(Calculation.log("signal3-changes-source", calculations))
+      }
+      .map(Calculation.log("signal3", calculations))
+
+    val signal4 = signal1
+      .composeChanges { signal1Changes =>
+        signal1Changes
+          .delaySync(signal3.changes)
+          .map(Calculation.log("signal4-changes-source", calculations))
+      }
+      .map(Calculation.log("signal4", calculations))
+
+    val signal5 = signal2
+      .composeChanges { signal2Changes =>
+        signal2Changes
+          .delaySync(signal3.changes)
+          .map(Calculation.log("signal5-changes-source", calculations))
+      }
+      .map(Calculation.log("signal5", calculations))
+
+    // --
+
+    // Order is important
+    signal5.addObserver(Observer.empty)
+    signal4.addObserver(Observer.empty)
+    signal3.addObserver(Observer.empty)
+
+    // signal4's and signal5's initial value does not depend on signal3's initial value,
+    // only on its changes, so it's ok that signal3's initial value is evaluated later.
+    assert(calculations.toList == List(
+      Calculation("signal1", 0),
+      Calculation("signal2", 0),
+      Calculation("signal5", 0),
+      Calculation("signal4", 0),
+      Calculation("signal3", 0)
+    ))
+
+    calculations.clear()
+
+    // --
+
+    v.set(1)
+
+    assert(calculations.toList == List(
+      Calculation("signal1", 1),
+      Calculation("signal2", 1),
+      Calculation("signal3-changes-source", 1),
+      Calculation("signal3", 1),
+      Calculation("signal5-changes-source", 1),
+      Calculation("signal5", 1),
+      Calculation("signal4-changes-source", 1),
+      Calculation("signal4", 1)
+    ))
+
+    calculations.clear()
+
+    // --
+
+    v.set(2)
+
+    assert(calculations.toList == List(
+      Calculation("signal1", 2),
+      Calculation("signal2", 2),
+      Calculation("signal3-changes-source", 2),
+      Calculation("signal3", 2),
+      Calculation("signal5-changes-source", 2),
+      Calculation("signal5", 2),
+      Calculation("signal4-changes-source", 2),
+      Calculation("signal4", 2)
+    ))
+
+    calculations.clear()
+
+  }
 }