Merge pull request #887 from square/zachklipp/skip-onpropschanged

zach-klippenstein · web-flow · commit a643b94cdf49 · 2020-01-23T17:06:48.000-08:00
Don't onPropsChanged unless the old and new props are actually unequal.
diff --git a/kotlin/workflow-runtime/src/main/java/com/squareup/workflow/internal/WorkflowNode.kt b/kotlin/workflow-runtime/src/main/java/com/squareup/workflow/internal/WorkflowNode.kt
@@ -248,9 +248,11 @@ internal class WorkflowNode<PropsT, StateT, OutputT : Any, RenderingT>(
     workflow: StatefulWorkflow<PropsT, StateT, OutputT, RenderingT>,
     newProps: PropsT
   ) {
-    val newState = workflow.onPropsChanged(lastProps, newProps, state)
-    diagnosticListener?.onPropsChanged(diagnosticId, lastProps, newProps, state, newState)
-    state = newState
+    if (newProps != lastProps) {
+      val newState = workflow.onPropsChanged(lastProps, newProps, state)
+      diagnosticListener?.onPropsChanged(diagnosticId, lastProps, newProps, state, newState)
+      state = newState
+    }
     lastProps = newProps
   }
 
diff --git a/kotlin/workflow-runtime/src/test/java/com/squareup/workflow/internal/WorkflowDiagnosticListenerIntegrationTest.kt b/kotlin/workflow-runtime/src/test/java/com/squareup/workflow/internal/WorkflowDiagnosticListenerIntegrationTest.kt
@@ -34,7 +34,6 @@ import kotlinx.coroutines.cancel
 import kotlinx.coroutines.cancelChildren
 import kotlinx.coroutines.channels.Channel
 import kotlinx.coroutines.flow.consumeAsFlow
-import kotlinx.coroutines.flow.flowOf
 import kotlinx.coroutines.flow.launchIn
 import kotlinx.coroutines.flow.map
 import kotlinx.coroutines.flow.produceIn
@@ -74,12 +73,11 @@ class WorkflowDiagnosticListenerIntegrationTest {
       spec = Spec(state = "root state")
 
       // Initial events.
-      assertEquals("state: root state\n", renderings.receive())
+      assertEquals("state: initial state\n", renderings.receive())
       assertEquals(
           listOf(
               "onWorkflowStarted",
               "onBeforeRenderPass",
-              "onPropsChanged",
               "onBeforeWorkflowRendered",
               "onAfterWorkflowRendered",
               "onAfterRenderPass",
@@ -118,7 +116,6 @@ class WorkflowDiagnosticListenerIntegrationTest {
               "onPropsChanged",
               "onBeforeWorkflowRendered",
               "onWorkflowStarted",
-              "onPropsChanged",
               "onBeforeWorkflowRendered",
               "onAfterWorkflowRendered",
               "onAfterWorkflowRendered",
@@ -139,11 +136,9 @@ class WorkflowDiagnosticListenerIntegrationTest {
               "onBeforeRenderPass",
               "onPropsChanged",
               "onBeforeWorkflowRendered",
-              "onPropsChanged",
               "onBeforeWorkflowRendered",
               "onAfterWorkflowRendered",
               "onWorkflowStarted",
-              "onPropsChanged",
               "onBeforeWorkflowRendered",
               "onAfterWorkflowRendered",
               "onAfterWorkflowRendered",
@@ -162,7 +157,6 @@ class WorkflowDiagnosticListenerIntegrationTest {
               "onBeforeRenderPass",
               "onPropsChanged",
               "onBeforeWorkflowRendered",
-              "onPropsChanged",
               "onBeforeWorkflowRendered",
               "onAfterWorkflowRendered",
               "onAfterWorkflowRendered",
@@ -185,17 +179,18 @@ class WorkflowDiagnosticListenerIntegrationTest {
     }
   }
 
-  @UseExperimental(ExperimentalCoroutinesApi::class)
+  @UseExperimental(ExperimentalCoroutinesApi::class, FlowPreview::class)
   @Test fun `workflow updates emit events in order`() {
+    val propsChannel = Channel<String>(1).apply { offer("initial props") }
     val channel = Channel<String>()
     val worker = channel.asWorker()
     fun action(value: String) = action<Nothing, String> { setOutput("output:$value") }
-    val workflow = Workflow.stateless<Unit, String, Unit> {
+    val workflow = Workflow.stateless<String, String, Unit> {
       runningWorker(worker, "key", ::action)
     }
 
     runBlocking {
-      launchWorkflowIn(this, workflow, flowOf(Unit)) { session ->
+      launchWorkflowIn(this, workflow, propsChannel.consumeAsFlow()) { session ->
         session.diagnosticListener = listener
             .andThen(SimpleLoggingDiagnosticListener())
         session.renderingsAndSnapshots.launchIn(this)
@@ -208,20 +203,11 @@ class WorkflowDiagnosticListenerIntegrationTest {
               "onRuntimeStarted",
               "onWorkflowStarted",
               "onBeforeRenderPass",
-              "onPropsChanged",
               "onBeforeWorkflowRendered",
               "onWorkerStarted",
               "onAfterWorkflowRendered",
               "onAfterRenderPass",
               "onBeforeSnapshotPass",
-              "onAfterSnapshotPass",
-              "onPropsChanged",
-              "onBeforeRenderPass",
-              "onPropsChanged",
-              "onBeforeWorkflowRendered",
-              "onAfterWorkflowRendered",
-              "onAfterRenderPass",
-              "onBeforeSnapshotPass",
               "onAfterSnapshotPass"
           ),
           actual = listener.consumeEventNames()
@@ -235,6 +221,21 @@ class WorkflowDiagnosticListenerIntegrationTest {
               "onWorkerOutput",
               "onWorkflowAction",
               "onBeforeRenderPass",
+              "onBeforeWorkflowRendered",
+              "onAfterWorkflowRendered",
+              "onAfterRenderPass",
+              "onBeforeSnapshotPass",
+              "onAfterSnapshotPass"
+          ), listener.consumeEventNames()
+      )
+
+      propsChannel.send("new props")
+      yield()
+      yield()
+      assertEquals(
+          listOf(
+              "onPropsChanged",
+              "onBeforeRenderPass",
               "onPropsChanged",
               "onBeforeWorkflowRendered",
               "onAfterWorkflowRendered",
diff --git a/kotlin/workflow-runtime/src/test/java/com/squareup/workflow/internal/WorkflowNodeTest.kt b/kotlin/workflow-runtime/src/test/java/com/squareup/workflow/internal/WorkflowNodeTest.kt
@@ -94,17 +94,30 @@ class WorkflowNodeTest {
 
   private val context: CoroutineContext = Unconfined
 
-  @Test fun `props are passed to on changed`() {
+  @Test fun `onPropsChanged is called when props change`() {
     val oldAndNewProps = mutableListOf<Pair<String, String>>()
     val workflow = PropsRenderingWorkflow { old, new, state ->
       oldAndNewProps += old to new
-      state
+      return@PropsRenderingWorkflow state
     }
-    val node = WorkflowNode(workflow.id(), workflow, "foo", null, context)
+    val node = WorkflowNode(workflow.id(), workflow, "old", null, context)
+
+    node.render(workflow, "new")
+
+    assertEquals(listOf("old" to "new"), oldAndNewProps)
+  }
+
+  @Test fun `onPropsChanged is not called when props are equal`() {
+    val oldAndNewProps = mutableListOf<Pair<String, String>>()
+    val workflow = PropsRenderingWorkflow { old, new, state ->
+      oldAndNewProps += old to new
+      return@PropsRenderingWorkflow state
+    }
+    val node = WorkflowNode(workflow.id(), workflow, "old", null, context)
 
-    node.render(workflow, "foo2")
+    node.render(workflow, "old")
 
-    assertEquals(listOf("foo" to "foo2"), oldAndNewProps)
+    assertTrue(oldAndNewProps.isEmpty())
   }
 
   @Test fun `props are rendered`() {
@@ -690,13 +703,13 @@ class WorkflowNodeTest {
     )
     listener.consumeEvents()
 
-    node.render(workflow.asStatefulWorkflow(), "props")
+    node.render(workflow.asStatefulWorkflow(), "new props")
 
     assertEquals(
         listOf(
-            "onPropsChanged(${node.diagnosticId}, props, props, (props:), (props:props:(props:)))",
-            "onBeforeWorkflowRendered(${node.diagnosticId}, props, (props:props:(props:)))",
-            "onAfterWorkflowRendered(${node.diagnosticId}, ((props:props:(props:)):props))"
+            "onPropsChanged(${node.diagnosticId}, props, new props, (props:), (props:new props:(props:)))",
+            "onBeforeWorkflowRendered(${node.diagnosticId}, new props, (props:new props:(props:)))",
+            "onAfterWorkflowRendered(${node.diagnosticId}, ((props:new props:(props:)):new props))"
         ), listener.consumeEvents()
     )
   }
@@ -728,7 +741,7 @@ class WorkflowNodeTest {
         parentDiagnosticId = 42,
         diagnosticListener = listener
     )
-    node.render(workflow.asStatefulWorkflow(), "props")
+    node.render(workflow.asStatefulWorkflow(), "new props")
     listener.consumeEvents()
 
     runBlocking {
@@ -751,8 +764,8 @@ class WorkflowNodeTest {
     )
     assertEquals(
         listOf(
-            "onWorkflowAction(${node.diagnosticId}, TestAction, (props:props:(props:))," +
-                " (props:props:(props:)), action output)"
+            "onWorkflowAction(${node.diagnosticId}, TestAction, (props:new props:(props:))," +
+                " (props:new props:(props:)), action output)"
         ), listener.consumeEvents()
     )
   }
@@ -784,7 +797,7 @@ class WorkflowNodeTest {
         parentDiagnosticId = 42,
         diagnosticListener = listener
     )
-    val rendering = node.render(workflow.asStatefulWorkflow(), "props")
+    val rendering = node.render(workflow.asStatefulWorkflow(), "new props")
     listener.consumeEvents()
 
     runBlocking {
@@ -804,7 +817,7 @@ class WorkflowNodeTest {
     assertEquals(
         listOf(
             "onSinkReceived(${node.diagnosticId}, TestAction)",
-            "onWorkflowAction(${node.diagnosticId}, TestAction, (props:props:(props:))," +
+            "onWorkflowAction(0, TestAction, (props:new props:(props:))," +
                 " state: foo, output: foo)"
         ), listener.consumeEvents()
     )
diff --git a/kotlin/workflow-tracing/src/test/resources/com/squareup/workflow/diagnostic/tracing/expected_trace_file.txt b/kotlin/workflow-tracing/src/test/resources/com/squareup/workflow/diagnostic/tracing/expected_trace_file.txt
