Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DNM: WIP: Keep the same RenderContext through Interceptor #991

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

steve-the-edwards
Copy link
Contributor

As part of #988 I also want to plumb the same RenderContext instance through the WorkflowInterceptor.

What we can do for this is simply save the RenderContext the first time it is used in render() on the Workflow that is created to accomplish the intercepting (via intercept()).

That is great and all, but we actually re-create this intercepting Workflow each time each of the APIs on WorkflowNode is called. So if we want to re-use the same RenderContext we also have to re-use the same intercepted Workflow. We can create this once at the init of the WorkflowNode since we have it's Workflow and then re-use it throughout.

However, if we do that then we need to ensure that we keep using that same Workflow throughout the lifetime of the WorkflowNode.

In other words, we have a 1 : 1 : 1 : 1 relationship between WorkflowNode : StatefulWorkflow : Intercepted Workflow : RenderContext. We don't recreate some combination of the last 3 each time, and we don't need to pass in the 2nd - the workflow itself each time we ask the WorkflowNode to render. It already has this saved.

Now, thankfully our excellent test coverage caught that this breaks one behavior - 'operators' on Workflows, such as mapRendering. With mapRendering we actually ask the same WorkflowNode to render a new Workflow - the one which we have mapped with the mapRendering transform. To do this, we actually create a new Workflow instance but we pass it to the same WorkflowNode to render this modified instance. If we go forward with these changes we can no longer do that. See the failing test mapRendering_with_same_upstream_workflow_in_two_different_passes_does_not_restart

@zach-klippenstein - do you know why we cared to do that in the first place? It seems confusing to me as it violates the 'natural' mental model of the 1:1 mapping between WorkflowNode and Workflow. I can see the use case for being able to map/transform the renderings of a Workflow without needing a new node but I don't see why we can't just encode that as a function that gets called on the rendering produced rather than the workflow?

@rjrjr I'm sure you'll have thoughts on this when returning from vacation, so leaving tagging you in this note for that time (and no sooner!).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants