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

Factor out a common parent interface for StatelessWorkflow and Workflow (now StatefulWorkflow). #219

Merged
merged 1 commit into from
Mar 27, 2019

Conversation

zach-klippenstein
Copy link
Collaborator

@zach-klippenstein zach-klippenstein commented Mar 27, 2019

tl;dr:

  • Workflow -> StatefulWorkflow
  • Workflow is an interface, StatefulWorkflow and StatelessWorkflow are abstract classes
    that both implement Workflow.

This change makes it much easier for consumers of this library to define public APIs with private
implementations. The public API is an interface that extends Workflow. The private
implementation is a class that subclasses either StatefulWorkflow or StatelessWorkflow,
depending on whether it needs to keep track of any state. Either way, the workflow's state type
is kept as a completely private concern.

interface Workflow<I, O, R>

This is the main currency of this library. Workflows can compose
other Workflows. A Workflow is anything that can be expressed as a StatefulWorkflow.
It has a single method, asStatefulWorkflow(), that is responsible for expressing the
workflow in terms of a StatefulWorkflow – a workflow that has an internal state. This
commit renames what used to be Workflow to StatefulWorkflow.

abstract class StatefulWorkflow<I, S, O, R>

This is what you subclass to write a Workflow that has internal state that persists
as long as the workflow is active in its parent. It knows about things like initial state
and snapshotting/restoring.

abstract class StatelessWorkflow<I, O, R>

This is what you subclass to get a Workflow that only has a single compose method.
It can compose children workflows, transform inputs/outputs/renderings up/down the tree,
and listen to external events and subscriptions, but it has no state.

Closes #213.

Based on #214

Internal discussion

@zach-klippenstein zach-klippenstein force-pushed the zachklipp/statefulworkflow branch 2 times, most recently from 0cf717b to 33bf938 Compare March 27, 2019 17:57
@zach-klippenstein zach-klippenstein added this to the v0.10.0 milestone Mar 27, 2019
Copy link
Contributor

@rjrjr rjrjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this.

@zach-klippenstein zach-klippenstein force-pushed the zachklipp/remove-compose-state-type branch from a2b675f to 0945a4c Compare March 27, 2019 20:06
@zach-klippenstein zach-klippenstein force-pushed the zachklipp/statefulworkflow branch 2 times, most recently from 84df51c to bf271ca Compare March 27, 2019 20:18
@zach-klippenstein zach-klippenstein changed the title WIP: Factor out a common parent interface for StatelessWorkflow and Workflow (now StatefulWorkflow). Factor out a common parent interface for StatelessWorkflow and Workflow (now StatefulWorkflow). Mar 27, 2019
@zach-klippenstein
Copy link
Collaborator Author

Fleshed out the docs, added a commit message, this is now ready for real review. When it's ready, I'll squash the first commit out and decline #214.

cc @rjrjr

Copy link
Contributor

@rjrjr rjrjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OMFG :shipit:

Just some doc suggestions.

> : Workflow<InputT, OutputT, RenderingT> {

/**
* Called when the state machine is first started to get the initial state.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Called from WorkflowContext.compose when a state machine is first started…

abstract fun initialState(input: InputT): StateT

/**
* Called whenever [WorkflowContext.compose] is about to get a new [input][InputT] value, to allow
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Called from WorkflowContext.compose instead of initialState when a workflow is already running. This
allows the workflow to detect changes in input, and possibly change its state in response.

* Called at least once† any time one of the following things happens:
* - This workflow's [input] changes (via the parent passing a different one in).
* - This workflow's [state] changes.
* - A child workflow's state changes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of A child workflow's state changes, how about A child workflow emits output, or updates its rendering.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But state changes do trigger composition, and rendering changes don't (because "render" is just another word for this method). Updating to include both state and output at any rate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right right.

* - This workflow's [state] changes.
* - A child workflow's state changes.
*
* **Never call this method directly.** To get the rendering from a child workflow, pass the child
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To nest the rendering of a child workflow in your own,

*
* @see snapshotState
*/
abstract fun restoreState(snapshot: Snapshot): StateT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #220

/**
* Called at least once any time one of the following things happens:
* - This workflow's [input] changes (via the parent passing a different one in).
* - A child workflow updates in some way (e.g. a stateful child's state changes).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/_(e.g.…)/(i.e., it emits output or updates its rendering)/

And add a bullet about workers?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would you like to see about workers? That term doesn't exist in any of the v2 code yet. I've updated this comment to match the updated wording for StatefulWorkflow.

Copy link
Contributor

@rjrjr rjrjr Mar 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That term doesn't exist in any of the v2 code yet Excellent, one less term!

Does seem like we should say something about the non-workflow / anonymous workflow calls on context. Whatever, we'll go over these docs again and again.

* @see snapshotState
*/
fun restoreState(snapshot: Snapshot): StateT
fun asStatefulWorkflow(): StatefulWorkflow<InputT, *, OutputT, RenderingT>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provides a StatefulWorkflow view of the receiver. Necessary because StatefulWorkflow is the common API required for WorkflowContext.compose() to do its work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this is why i always leave TODO kdoc comments everywhere…

// @formatter:on
context.compose(this@mapRendering, input) { emitOutput(it) }
.let(transform)
}

/**
* Returns a rendering that directly delegates to this [Workflow] but masks its state type.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥

@zach-klippenstein zach-klippenstein force-pushed the zachklipp/statefulworkflow branch from bf271ca to 263002c Compare March 27, 2019 21:49
@zach-klippenstein zach-klippenstein changed the base branch from zachklipp/remove-compose-state-type to master March 27, 2019 21:50
@zach-klippenstein zach-klippenstein force-pushed the zachklipp/statefulworkflow branch from 263002c to b1f52c7 Compare March 27, 2019 21:52
@zach-klippenstein zach-klippenstein force-pushed the zachklipp/statefulworkflow branch 2 times, most recently from f236ad4 to 4a1a6c7 Compare March 27, 2019 22:06
…ow (now StatefulWorkflow).

tl;dr:
 - `Workflow` -> `StatefulWorkflow`
 - `Workflow` is an interface, `StatefulWorkflow` and `StatelessWorkflow` are abstract classes
   that both implement `Workflow`.

This change makes it much easier for consumers of this library to define public APIs with private
implementations. The public API is an interface that extends `Workflow`. The private
implementation is a class that subclasses either `StatefulWorkflow` or `StatelessWorkflow`,
depending on whether it needs to keep track of any state. Either way, the workflow's state type
is kept as a completely private concern.

`interface Workflow<I, O, R>`
-----------------------------

This is the main currency of this library. `Workflow`s can compose
other `Workflow`s. A `Workflow` is anything that can be expressed as a `StatefulWorkflow`.
It has a single method, `asStatefulWorkflow()`, that is responsible for expressing the
workflow in terms of a `StatefulWorkflow` – a workflow that has an internal state. This
commit renames what used to be `Workflow` to `StatefulWorkflow`.

`abstract class StatefulWorkflow<I, S, O, R>`
---------------------------------------------

This is what you subclass to write a `Workflow` that has internal state that persists
as long as the workflow is active in its parent. It knows about things like initial state
and snapshotting/restoring.

`abstract class StatelessWorkflow<I, O, R>`
-------------------------------------------

This is what you subclass to get a `Workflow` that only has a single `compose` method.
It can compose children workflows, transform inputs/outputs/renderings up/down the tree,
and listen to external events and subscriptions, but it has no state.

Closes #213.
@zach-klippenstein zach-klippenstein force-pushed the zachklipp/statefulworkflow branch from 4a1a6c7 to 44b3bc7 Compare March 27, 2019 22:14
@zach-klippenstein
Copy link
Collaborator Author

zach-klippenstein commented Mar 27, 2019

Thanks for the merge fixes @rjrjr 😁 Gonna merge when CI goes green.

@zach-klippenstein
Copy link
Collaborator Author

Travis is having issues:
image

I ran ./gradlew clean connectedCheck locally, everything passed, so I'm just gonna merge.

@zach-klippenstein zach-klippenstein merged commit caf4f92 into master Mar 27, 2019
@zach-klippenstein zach-klippenstein deleted the zachklipp/statefulworkflow branch March 27, 2019 22:20
@zach-klippenstein zach-klippenstein added the kotlin Affects the Kotlin library. label Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kotlin Affects the Kotlin library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WorkflowContext.compose shouldn't have to know child workflow's state type.
2 participants