Add Interceptor Methods for Skip and Update Rendering #1297
Conversation
9f2152b to
8f6d19e
Compare
| * as well as two extra parameters: | ||
| * This interface's methods mirror the methods of [StatefulWorkflow], and three additional methods, | ||
| * explained below. | ||
| * Each method returns the same thing as the corresponding method on [StatefulWorkflow], and\ |
| @@ -129,6 +142,21 @@ public interface WorkflowInterceptor { | |||
| session: WorkflowSession | |||
| ): Snapshot? = proceed(state) | |||
|
|
|||
| / 8000 ** | |||
| * Called when a [RenderingAndSnapshot] is updated from the runtime (i.e. when the [StateFlow] | |||
| * is provided a new value - this may still be de-duplicated). | |||
There was a problem hiding this comment.
"this may still be de-duplicated" => by consumers of the state flow, based on some other things?
There was a problem hiding this comment.
by the StateFlow itself which de-duplicates emissions downstream.
| /** | ||
| * Called when the rendering is skipped because no state has changed after action cascade(s). | ||
| */ | ||
| public fun onRenderingSkipped(): Unit = Unit |
There was a problem hiding this comment.
if the outcome of an action loop is always either "onRenderingSkipped" or "onRenderingUpdated", then I would actually merge those and pass in a sealed class of "Updated" vs "Skipped", which would make this behavior a lot clearer.
There was a problem hiding this comment.
ya I think that sounds reasonable!
| * | ||
| * @return [RenderingAndSnapshot]: a possibly modified [RenderingAndSnapshot]. | ||
| */ | ||
| public fun <R> onRenderingUpdated( |
There was a problem hiding this comment.
It's not clear from the name that this only applies to the rendering published out to the external consumer. Combined with Py's suggestion below, could we name it something like "onExternalRenderingEvent" (with an updated/skipped parameter).
There was a problem hiding this comment.
yes I can rename this.
| * 1. [onRenderingSkipped] - called when the rendering is short-circuited because no state | ||
| * change was detected as part of the action cascade(s) and the | ||
| * [RuntimeConfig.RENDER_ONLY_WHEN_STATE_CHANGES] is enabled. | ||
| * 1. [onRenderingUpdated] - called when a [RenderingAndSnapshot] is passed to the [StateFlow] | ||
| * returned by [renderWorkflowIn]. It is passed the [RenderingAndSnapshot] which could be | ||
| * decorated. |
There was a problem hiding this comment.
This feels a bit awkward to me. Up to this point, everything in WorkflowInterceptor is 1-1 with every individual workflow in the system. These new methods, IIUC, apply to the runtime as a whole. I feel like it violates SRP? I don't want to be overly pedantic about it and I don't have any real practical reason to not put them here.
I do wonder if this would be a good excuse to figure out a dedicated, higher-level tracing API though like we'd need for compose-based workflows. I can jump on that this week a bit probably, although I'll be leaving on Friday for 2 weeks.
There was a problem hiding this comment.
onSessionStarted actually existed before these changes (I just moved the documentation and made it clear).
Yes, agreed that we are violating SRP and there is an opening here for a higher/orthogonal tracing dedicated API. I also don't want to tackle that with this work if possible and the interceptor can handle this without growing any more unwieldy than it already is, I think.
This is necessary in order to do effective tracing with optimizations enabled.
These are also helpful and quite logical methods for the interceptor.
Specifically
onRenderingUpdated(rendering)is powerful as you could decorate every rendering passed out of the runtime via an interceptor.