985: render() again only if state has been changed by the action (or its parent's output handling) #992

steve-the-edwards · 2023-04-20T22:00:44Z

aiming to satisfy #985 .

After applying an action, we add another layer of information to what we pass to the runtime. This is the ActionApplied data class which includes both the output and whether or not the state was changed.

public data class ActionApplied<out OutputT>(
  public val output: WorkflowOutput<OutputT>?,
  public val stateChanged: Boolean = false,
) : ActionProcessingResult

If output is produced, we pass it up the parent immediately (as before), but keep the stateChanged value sticky if it has changed for any child.

This is how it is done in WorkflowNode:

  private fun applyAction(
    action: WorkflowAction<PropsT, StateT, OutputT>,
    childResult: ActionApplied<*>? = null
  ): ActionProcessingResult {
    val (newState, actionApplied) = action.applyTo(lastProps, state)
    state = newState
    // Aggregate the action with the child result, if any.
    val aggregateActionApplied = actionApplied.copy(
      // Changing state is sticky, we pass it up if it ever changed.
      stateChanged = actionApplied.stateChanged || (childResult?.stateChanged ?: false)
    )
    return if (actionApplied.output != null) {
      emitAppliedActionToParent(aggregateActionApplied)
    } else {
      aggregateActionApplied
    }
  }

In the runtime loop then, we look at the ActionProcessingResult from the select statement and if it is an ActionApplied we can choose whether or not to render based on whether stateChanged == true.

We also set up a RuntimeConfig extension - renderingOnStateChangeOnly to turn this logic on in the runtime. It also adds tests specifically for it and new shards to run all of our tests against this runtime.

workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/WorkflowNode.kt

workflow-core/src/commonMain/kotlin/com/squareup/workflow1/WorkflowAction.kt

gradle.properties

...dungeon/timemachine/src/test/java/com/squareup/sample/timemachine/TimeMachineWorkflowTest.kt

...nfig/config-android/src/main/java/com/squareup/workflow1/config/AndroidRuntimeConfigTools.kt

rjrjr · 2023-05-16T23:31:28Z

workflow-core/src/commonMain/kotlin/com/squareup/workflow1/WorkflowAction.kt

+    public fun forceRerender() {
+      forcedRerender = true


Surprised this exists, and surprised that it's public. Hard to imagine feature devs putting this to anything but bad uses.

I originally did not have this. I've now added this as in internal testing we have a number of scenarios where we drive 'other' (non-Workflow) UI via side effects that occur in actions, but then that require a re-render() to pick up the changes in that other UI. So either we add some kind of increasing counter to the state for those cases or we add a hook to force re-render() without state change like this. I wanted to just test with this first. Sorry I'm realizing now that this PR shoudl still be in draft. I'm not sure why it wasn't. Probably I was thinking at some point it was more ready than it is.

Removed this until we decide this is the strategy we want to handle non-Workflow integrations.

workflow-core/src/commonMain/kotlin/com/squareup/workflow1/WorkflowAction.kt

rjrjr

Nothing but nits. So excited!

rjrjr · 2023-06-08T20:02:00Z

.github/workflows/kotlin.yml

@@ -188,6 +188,62 @@ jobs :
        with :
          report_paths : '**/build/test-results/test/TEST-*.xml'

+  jvm-stateChange-runtime-test :


Woot! @RBusarow should own reviewing this for correctness, though.

i did do some cleanup after this - #1011

rjrjr · 2023-06-08T20:06:55Z

...nfig/config-android/src/main/java/com/squareup/workflow1/config/AndroidRuntimeConfigTools.kt

+        "conflate" -> ConflateStaleRenderings(renderOnStateChangeOnly = false)
+        "conflate-stateChange" -> ConflateStaleRenderings(renderOnStateChangeOnly = true)
+        "baseline-stateChange" -> RenderPerAction(renderOnStateChangeOnly = true)
+        else -> RuntimeConfig.DEFAULT_CONFIG


Seems like we should be a bit more strict so that we catch typos for people.

Suggested change

"conflate" -> ConflateStaleRenderings(renderOnStateChangeOnly = false)

"conflate-stateChange" -> ConflateStaleRenderings(renderOnStateChangeOnly = true)

"baseline-stateChange" -> RenderPerAction(renderOnStateChangeOnly = true)

else -> RuntimeConfig.DEFAULT_CONFIG

"conflate" -> ConflateStaleRenderings(renderOnStateChangeOnly = false)

"conflate-stateChange" -> ConflateStaleRenderings(renderOnStateChangeOnly = true)

"baseline-stateChange" -> RenderPerAction(renderOnStateChangeOnly = true)

"", "baseline" -> RuntimeConfig.DEFAULT_CONFIG

else -> throw IllegalArgumentException("Unrecognized config \"${BuildConfig.WORKFLOW_RUNTIME}\"")

If you take the enum set suggestion below, we could retire --runtime-config and replace it with three separate boolean flags.

I like the Set idea!

I'm going to leave it as strings for the options though as I don't want to mess with the buildConfig stuff I set up before (the command line options are easy - but the buildconfig for the instrumentation tests is harder IIRC)

rjrjr · 2023-06-08T20:10:43Z

workflow-core/src/commonMain/kotlin/com/squareup/workflow1/WorkflowAction.kt

+ *   which could be null.
+ * @param stateChanged: whether or not the action changed the state.
+ */
+public data class ActionApplied<out OutputT>(


Do we actually rely on this being a data class? I remember that being a landmine when used in public API, though I forget exactly why.

Remember that you can implement positional params manually, it's pretty trivial.

Like to support destructuring you mean?

Hmm, now I'm curious - what the was the data class issue in the public API?

We do use a .copy (which I know I could implement). However, since I've done all the testing and verification with it as a data class I'm loathe to change that.

I have a vague recollection that we had issues before with a data class based on how it does equality? We should be able to rely on that here though.

For posterity. I am going to do this change after all. Canonical resource describing backwards compatibility concerns: https://jakewharton.com/public-api-challenges-in-kotlin/

rjrjr · 2023-06-08T20:16:07Z

workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/RenderWorkflow.kt

+  /**
+   * We only check for this when [RuntimeConfig.renderOnStateChangeOnly] is true.
+   */
+  suspend fun maybeCheckNoStateChange(actionResult: ActionProcessingResult): Boolean {


How about shortCircuitForUnchangedState? And have the kdoc explain what should happen when true is returned.

Nice. Done.

/** * If [runtimeConfig] contains [RuntimeConfigOptions.RENDER_ONLY_WHEN_STATE_CHANGES] then * send any output, but return true which means restart the runtime loop and process another * action. */ suspend fun shortCircuitForUnchangedState(actionResult: ActionProcessingResult): Boolean { if (runtimeConfig.contains(RENDER_ONLY_WHEN_STATE_CHANGES) && actionResult is ActionApplied<*> && !actionResult.stateChanged ) { // Possibly send output and process more actions. No state change so no re-render. sendOutput(actionResult, onOutput) return true } return false }

"and process another action" seems to imply that we will render, just that we're going to try to process more actions first. Should this say "and process the next action (if any)"?

Just missed me merging this :). But I will follow up with this change.

Technically 'processing an action' can involve suspending until there is one but that's more than a little bit confusing.

rjrjr · 2023-06-08T20:23:01Z

workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/RuntimeConfig.kt

+   * If state has not changed from an action cascade, do not re-render. This has been mostly
+   * proven out. However, be careful if you have any non-Workflow code you integrate with that
+   * depends on the Workflow tree re-rendering to pick up changes from its equivalent 'view model.'
+   * You should change some kind of Workflow state when updating that external code if you want
+   * Workflow to pick up the change and render again.


Suggested change

* If state has not changed from an action cascade, do not re-render. This has been mostly

* proven out. However, be careful if you have any non-Workflow code you integrate with that

* depends on the Workflow tree re-rendering to pick up changes from its equivalent 'view model.'

* You should change some kind of Workflow state when updating that external code if you want

* Workflow to pick up the change and render again.

* If state has not changed from an action cascade (as determined via `equals()`),

* do not re-render. For example, when this is true and `noAction()` is enqueued,

* the current `render()` pass will short circuit and no rendering will be posted

* through the `StateFlow` returned from `renderWorkflowIn()`.

*

* This has been mostly proven out. However, be careful if you have any non-Workflow

* code you integrate with that depends on the Workflow tree re-rendering to pick up

* changes from its equivalent 'view model.' You should change some kind of Workflow

* state when updating that external code if you want Workflow to pick up the change

* and render again.

Used in the updated location in the enum.

rjrjr · 2023-06-08T20:25:03Z

workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/WorkflowNode.kt

+    action: WorkflowAction<PropsT, StateT, OutputT>,
+    childResult: ActionApplied<*>? = null
+  ): ActionProcessingResult {
+    val (newState, actionApplied) = action.applyTo(lastProps, state)


Suggested change

val (newState, actionApplied) = action.applyTo(lastProps, state)

val (newState: StateT, actionApplied: ActionApplied<StateT>) = action.applyTo(lastProps, state)

Not positive about the ActionApplied type, but you get the idea.

rjrjr · 2023-06-08T20:27:46Z

workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/RenderWorkflowInTest.kt

-    Pair(RenderPerAction, ConflateStaleRenderings),
-    Pair(ConflateStaleRenderings, RenderPerAction),
-    Pair(ConflateStaleRenderings, ConflateStaleRenderings),
+    Pair(RenderPerAction(), RenderPerAction()),


Holy combinatorial explosion! Would this get any simpler of the config was an enum set and RuntimeConfig was a three value enum? Baseline config would be an empty set.

Narrator: It didn't get any simpler.

We still likely want to test all combinations between the 2 workflowRuntimes we are launching. It's maybe overkill, but its not that slow so its worthwhile.

rjrjr · 2023-06-08T20:32:43Z

workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/internal/SubtreeManagerTest.kt

+    assertFalse(result.stateChanged)
+  }
+
+  @Test fun tick_children_handles_no_child_output() = runTest {


nit / out of scope: we don't have a tick() method any more, should we update test and var names here?

Good point. #1012

…r on State Change Only

steve-the-edwards force-pushed the sedwards/985-state-compare branch 3 times, most recently from 464ebff to 05285fb Compare April 24, 2023 17:47

steve-the-edwards changed the title ~~WIP: 985: Add ability to propagate state comparison up tree~~ 985: Add ability to propagate state comparison up tree Apr 24, 2023

steve-the-edwards marked this pull request as ready for review April 24, 2023 18:42

steve-the-edwards requested review from a team and zach-klippenstein as code owners April 24, 2023 18:42

steve-the-edwards mentioned this pull request Apr 24, 2023

Offer config option to skip rendering when new state == old #985

Closed

steve-the-edwards force-pushed the sedwards/985-state-compare branch 2 times, most recently from e65278a to 25e65d7 Compare April 25, 2023 17:46

steve-the-edwards commented Apr 28, 2023

View reviewed changes

workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/WorkflowNode.kt Outdated Show resolved Hide resolved

workflow-core/src/commonMain/kotlin/com/squareup/workflow1/WorkflowAction.kt Outdated Show resolved Hide resolved

steve-the-edwards force-pushed the sedwards/985-state-compare branch 4 times, most recently from 26e95af to 1d2b00c Compare May 15, 2023 21:51

steve-the-edwards changed the title ~~985: Add ability to propagate state comparison up tree~~ 985: render() again only if state has been changed by the action (or its parent's output handling) May 16, 2023

steve-the-edwards force-pushed the sedwards/985-state-compare branch 2 times, most recently from cafac7e to 84ac726 Compare May 16, 2023 21:31

rjrjr reviewed May 16, 2023

View reviewed changes

steve-the-edwards force-pushed the sedwards/985-state-compare branch from 84ac726 to 2db3e12 Compare June 8, 2023 19:05

steve-the-edwards requested a review from rjrjr June 8, 2023 19:48

rjrjr approved these changes Jun 8, 2023

View reviewed changes

steve-the-edwards force-pushed the sedwards/985-state-compare branch from 2db3e12 to eefa11e Compare June 8, 2023 21:45

steve-the-edwards requested a review from a team as a code owner June 8, 2023 21:45

985: Add Ability to track whether state changed; Add Config for Rende…

19d02fc

…r on State Change Only

steve-the-edwards force-pushed the sedwards/985-state-compare branch from eefa11e to 19d02fc Compare June 9, 2023 14:51

steve-the-edwards merged commit 6446099 into main Jun 9, 2023

steve-the-edwards deleted the sedwards/985-state-compare branch June 9, 2023 15:57

steve-the-edwards mentioned this pull request Jun 9, 2023

Use RenderOnlyOnStateChange for RenderPassCounting #993

Closed

rjrjr mentioned this pull request Jun 15, 2023

Consider version of initialState that provides a CoroutineScope identical to the WorkflowNode's #990

Closed

steve-the-edwards mentioned this pull request Jun 19, 2023

Allow runningWorker / renderChild handlers to return null and avoid re-render. #915

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

985: render() again only if state has been changed by the action (or its parent's output handling) #992

985: render() again only if state has been changed by the action (or its parent's output handling) #992

steve-the-edwards commented Apr 20, 2023 •

edited

Loading

rjrjr May 16, 2023

steve-the-edwards May 17, 2023

steve-the-edwards Jun 8, 2023

rjrjr left a comment

rjrjr Jun 8, 2023

steve-the-edwards Jun 8, 2023

rjrjr Jun 8, 2023 •

edited

Loading

steve-the-edwards Jun 8, 2023

rjrjr Jun 8, 2023

rjrjr Jun 8, 2023

steve-the-edwards Jun 8, 2023

steve-the-edwards Jun 8, 2023

steve-the-edwards Jun 13, 2023

rjrjr Jun 8, 2023

steve-the-edwards Jun 8, 2023

rjrjr Jun 9, 2023 •

edited

Loading

steve-the-edwards Jun 9, 2023

steve-the-edwards Jun 9, 2023

rjrjr Jun 8, 2023

steve-the-edwards Jun 8, 2023

rjrjr Jun 8, 2023

rjrjr Jun 8, 2023

steve-the-edwards Jun 8, 2023

rjrjr Jun 8, 2023

steve-the-edwards Jun 8, 2023

-   * If state has not changed from an action cascade, do not re-render. This has been mostly
-   * proven out. However, be careful if you have any non-Workflow code you integrate with that
-   * depends on the Workflow tree re-rendering to pick up changes from its equivalent 'view model.'
-   * You should change some kind of Workflow state when updating that external code if you want
-   * Workflow to pick up the change and render again.
+   * If state has not changed from an action cascade (as determined via `equals()`),
+   * do not re-render. For example, when this is true and `noAction()` is enqueued,
+   * the current `render()` pass will short circuit and no rendering will be posted
+   * through the `StateFlow` returned from `renderWorkflowIn()`.
+   *
+   * This has been mostly proven out. However, be careful if you have any non-Workflow
+   * code you integrate with that depends on the Workflow tree re-rendering to pick up
+   * changes from its equivalent 'view model.' You should change some kind of Workflow
+   * state when updating that external code if you want Workflow to pick up the change
+   * and render again.

	val (newState, actionApplied) = action.applyTo(lastProps, state)
	val (newState: StateT, actionApplied: ActionApplied<StateT>) = action.applyTo(lastProps, state)

985: render() again only if state has been changed by the action (or its parent's output handling) #992

985: render() again only if state has been changed by the action (or its parent's output handling) #992

Conversation

steve-the-edwards commented Apr 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjrjr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjrjr Jun 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjrjr Jun 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steve-the-edwards commented Apr 20, 2023 •

edited

Loading

rjrjr Jun 8, 2023 •

edited

Loading

rjrjr Jun 9, 2023 •

edited

Loading