-
Notifications
You must be signed in to change notification settings - Fork 70
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
Remove knowledge of child StateT type from compose. #214
Conversation
@@ -38,7 +38,7 @@ import kotlin.coroutines.experimental.CoroutineContext | |||
* [Workflow.initialState]. | |||
*/ | |||
internal class WorkflowNode<InputT : Any, StateT : Any, OutputT : Any, RenderingT : Any>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If WorkflowContext.compose
no longer knows about StateT
, how is WorkflowNode
learning about it? Where does it come from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It basically doesn't, it's effectively Any
. But it doesn't need to in practice. The generic param is basically just there so the Workflow
methods all agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me see if I understand: the StateT
of the root WorkflowNode
comes from WorkflowHost.Factory#run$workflow_host
. But of every other WorkflowNode
in the tree has StateT
of Any
? Given that, can you push this PR farther and get rid of StateT
from WorkflowNode
and RealWorkflowContext
?
b77591f
to
a2b675f
Compare
return rendering | ||
} | ||
): RenderingT = | ||
composeWithStateType(workflow as Workflow<InputT, StateT, OutputT, RenderingT>, input) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rjrjr The cast you're looking for is here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay with me to merge as is to lock in the gain, but if we can push this further we really should.
a2b675f
to
0945a4c
Compare
Declining in favor of #219. |
Closes #213.