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

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

Closed
steve-the-edwards opened this issue Apr 13, 2023 · 9 comments · Fixed by #1106
Assignees
Labels
enhancement New feature or request

Comments

@steve-the-edwards
Copy link
Contributor

Then for example, a StateFlow operator could be launched from onInitialState under a scope identical to the workflow's scope.

@steve-the-edwards
Copy link
Contributor Author

Thinking about this more...

we have had 2 main use cases for a StateFlow :

  1. So that we could get .value out of the StateFlow in initialState to avoid one extra bootstrap re-rendering (for performance reasons). If we are going to do that stateIn in initialState then it either won't be synchronous (the suspending version) or we already have the correct initialValue synchronously (as props or otherwise), in which case we don't really need a StateFlow from initialState onwards. We can just populate the Workflow's state with the initial value and use the flow form a Worker in render().
  2. The 2nd case is if we have operators that we want to do on one or more upstream StateFlows. Given any operator has work to do and brings this back to a flow, we need to stateIn after the operator in order to get it back to a StateFlow to access .value synchronously in initialState. The operator combination is likely a concern of the Workflow usage only, so we want to be able to model the code for it there. It would be better to be able to do the operators and then a stateIn attached to the Workflow's lifecycle itself (thus going back to providing the CoroutineScope in initialState). However, as I think through this now I'm not sure this case doesn't just collapse into the first. If we have all upstream StateFlow signals we can .value them all already in initialState and then run the operator/combination logic synchronously (it better be fast) in initialState to get a value to populate our state without extra re-renders. Then we're happy enough if the operators result in a flow to collect from in a Worker.

So now I've convinced myself that I don't see a case where we need CoroutineScope in initialState to accomplish something we couldn't before. It might be a useful convenience, but I need to think about whether or not we want to enable that convenience vs. encouraging doing the above steps. 🤔

@rjrjr
Copy link
Contributor

rjrjr commented Jun 15, 2023

Case one is less interesting now that #992 has landed, but this still seems like it would solve a lot of other problems.

@rjrjr rjrjr changed the title Consider version of onInitialState that provides a CoroutineScope identical to the WorkflowNode's Consider version of initialState that provides a CoroutineScope identical to the WorkflowNode's Jun 15, 2023
@steve-the-edwards
Copy link
Contributor Author

still seems like it would solve a lot of other problems.

In terms of discoverability and having good tools 'at hand' you mean?

I feel like I've ruled out what I thought before were the main use cases in terms of being blocked without this.

@rjrjr
Copy link
Contributor

rjrjr commented Jun 15, 2023

You have a much clearer picture of the situation than I do. Let's definitely not build this until we have real people with real use cases in hand.

@steve-the-edwards steve-the-edwards added the enhancement New feature or request label Jun 19, 2023
@blakelee
Copy link
Collaborator

blakelee commented Jul 24, 2023

I have a case where I need a CoroutineScope that lasts for the duration of a workflow. I need a scope and object from a callback in order to create a new state. With the current implementation it's going to look like something like this on the render function

context.runningSideEffect("scope") {
  val scope = this
  context.actionSink.send(action {
    val newState = state.result?.let { NewState(NewObject(scope, it))}
    val oldState = state.copy(scope = scope)  
    state = newState ?: oldState
  })
  // suspend forever
}

return Rendering(
  onObjectCreated = context.eventHandler {
    val newState = state.scope?.let { NewState(NewObject(it, result))}
    val oldState = state.copy(result = result)
    state = newState ?: oldState
  }
)

This creates a funky race condition where I have to keep track of two callbacks and having a CoroutineScope available for the workflow would be very helpful.

@rjrjr
Copy link
Contributor

rjrjr commented Aug 31, 2023

From an offline conversation with @steve-the-edwards:

public abstract class SessionWorkflow<
  SessionT,
  in PropsT,
  StateT,
  out OutputT,
  out RenderingT
  > : Workflow<PropsT, OutputT, RenderingT>, IdCacheable {

  public abstract fun onSessionStarted(scope: CoroutineScope): SessionT

  public abstract fun initialState(
    session: SessionT,
    props: PropsT,
    snapshot: Snapshot?
  ): StateT

  public abstract fun render(
    session: SessionT,
    renderProps: PropsT,
    renderState: StateT,
    context: RenderContext
  ): RenderingT
public abstract class StatefulWorkflow<
  in PropsT,
  StateT,
  out OutputT,
  out RenderingT
  > : SessionWorkflow<Unit, PropsT, OutputT, RenderingT>, IdCacheable

  public final fun initialState(
    session: SessionT,
    props: PropsT,
    snapshot: Snapshot?
  ): StateT = initialState(props, snapshot)

  public abstract final initialState(
    props: PropsT,
    snapshot: Snapshot
  ): StateT

One big idea here is that SessionT could be a Dagger Component, built from Dagger modules that could include providers for a StateFlow whose lifetime needs to match that of a particular WorkflowNode. Any child workflow instantiated from that Component would be able get at that StateFlow, or, say, a repository that wraps it, via constructor injection. Or those children could constructor inject child workflows that in turn can inject that StateFlow

I like the idea of SessionWorkflow as a separate type because I think that 99% of Workflow implementations should not be thinking about SessionT directly. It's useful for high level lifecycle needs -- the LoggedInWorkflow, the SettingsAppletWorkflow. It's just noise for the average FooScreen leaf workflow.

@rjrjr
Copy link
Contributor

rjrjr commented Aug 31, 2023

So now the next question is: do we really need SessionT for this pattern? Or could LoggedInWorkflow simply manage its Dagger component as part of its StateT? I bet it could, and in fact we may have some apps doing exactly that already.

So did I just talk myself back into the original plan of simply adding a CoroutineScope param to initialState(), and advising folks to take advantage of this by adding a Dagger Component to StateT? Instance equality would prevent them from blowing up any optimizations.

@rjrjr
Copy link
Contributor

rjrjr commented Aug 31, 2023

Note that StateFlowImpl appears to use default (instance) equality.

@steve-the-edwards
Copy link
Contributor Author

So did I just talk myself back into the original plan of simply adding a CoroutineScope param to initialState()

Perhaps, because without a notion of SessionT or WorkflowLocal we wouldn't be able to pass any computed StateFlow from onSessionStarted through to the StateT so it would need to be in initialState

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

Successfully merging a pull request may close this issue.

3 participants