Add Conflate Stale Renderings Runtime #868
Conversation
steve-the-edwards
force-pushed
the
sedwards/runtime-prototypes
branch
from
b7536ef to
2030aef
Compare
steve-the-edwards
force-pushed
the
sedwards/proto-conflate
branch
3 times, most recently
from
14acb3f to
ae4d7fd
Compare
steve-the-edwards
requested review from
a team and
zach-klippenstein
as code owners
| actionResult: ActionProcessingResult?, | ||
| onOutput: suspend (OutputT) -> Unit | ||
| ): RenderingAndSnapshot<RenderingT> { | ||
| // After receiving an output from the actions that were processed, |
There was a problem hiding this comment.
Odd to have the same comment in two places (here and line 171). I think it's just confusing here.
There was a problem hiding this comment.
Removed and clarified.
| when (actionResult) { | ||
| is WorkflowOutput<*> -> { | ||
| @Suppress("UNCHECKED_CAST") | ||
| val output = actionResult as? WorkflowOutput<OutputT> | ||
| output?.let { onOutput(it.value) } | ||
| } | ||
| else -> {} // no -op | ||
| } |
There was a problem hiding this comment.
Does a single cast not work?
| when (actionResult) { | |
| is WorkflowOutput<*> -> { | |
| @Suppress("UNCHECKED_CAST") | |
| val output = actionResult as? WorkflowOutput<OutputT> | |
| output?.let { onOutput(it.value) } | |
| } | |
| else -> {} // no -op | |
| } | |
| @Suppress("UNCHECKED_CAST") | |
| (actionResult as? WorkflowOutput<OutputT>)?.let { | |
| onOutput(it.value) | |
| } |
There was a problem hiding this comment.
Changed to this.
| // After receiving an output, the next render pass must be done before emitting that output, | ||
| // so that the workflow states appear consistent to observers of the outputs and renderings. | ||
| renderingsAndSnapshots.value = runner.nextRendering() | ||
| output?.let { onOutput(it.value) } | ||
| nextRenderAndSnapshot = renderAndEmitOutput(runner, actionResult, onOutput) |
There was a problem hiding this comment.
We're violating what this comment warns about -- under ConflateStateRenderings, we may emit several outputs before the next rendering. Question is, does that matter? @zach-klippenstein and I convinced each other that it does, but I don't believe that any more -- sanity check requested.
If we do think it's true, then this method needs to exit immediately after emitting output, right? Perhaps it becomes a do {} while().
There was a problem hiding this comment.
IC what you mean. The wording needs more specificity. We are doing a Render Pass before emitting each output (they are tied together in the helper method).
What we are not doing is emitting the new rendering in the flow (and thus being able to update any UI).
You would likely have a better practical sense of use cases, but in theory the Output handling should be orthogonal to UI updates. Also we know that UI is already stale, so we wait to give you the fresh one. But Output by nature of being an event, can never be stale.
There was a problem hiding this comment.
// If the action did produce an Output, we send it immediately after the render pass.
nextRenderAndSnapshot = renderAndEmitOutput(runner, actionResult, onOutput)
if (runtimeConfig == ConflateStaleRenderings) {
// With this runtime modification, we do not pass renderings we know to be stale. This
// means that we may be calling onOutput out of sync with the update of the UI. Output
// is an event though, and should always occur immediately - i.e. it cannot be stale.
There was a problem hiding this comment.
The wording needs more specificity.
I think it was pretty clearly about emitting the rendering, since we're talking about observers.
But it sounds like you agree that it's okay to consider the output and rendering streams as separate things.
steve-the-edwards
force-pushed
the
sedwards/proto-conflate
branch
from
ae4d7fd to
aae212a
Compare
Check for empty channels and synchronously return if they are empty.
steve-the-edwards
force-pushed
the
sedwards/proto-conflate
branch
from
aae212a to
8fefe33
Compare
Performance tests for this new runtime are forthcoming, but not included in this PR.