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

Refactor Launch form to use a state machine #99

Merged
merged 10 commits into from
Oct 2, 2020

Conversation

schottra
Copy link
Contributor

@schottra schottra commented Sep 29, 2020

This migrates the state representations for LaunchWorkflowForm into a xstate state machine. Previously, a lot of logic and context variables were store in useLaunchWorkflowFormState. Now, all of the states in the flow are represented in launchMachine, which is consumed by the state hook.

The machine uses a handful of services to do the async work. These are provided by the state hook.

I also moved the state management for the workflow/launch plan selectors into a separate state hook. This is in preparation for making the launch form state generic enough to handle either a workflow or task as a source.

The net result of this should be no change in functionality.

Note: There are some TODOs left on this branch. This PR is against a feature branch, and I intend to address them in a follow-up PR.

  • Added launchMachine, which includes state definitions for schema, context, events for the whole launch flow. It also exports a machine config and a machine instance to be consumed by an interpreter.
  • Moved most of the context variables and state management out of useLaunchWorkflowFormState. It now only contains the services used by the state machine for loading workflows, launch plans, and inputs. It also wires the state machine context into a child instance of a useWorkflowSourceSelectorState hook.
  • Added useWorkflowSourceStateSelector hook, which translates between state machine context and the props/states needed for our SearchableSelector component.
  • Updated LaunchWorkflowForm to use state-matching for conditional rendering of elements, and to consume the various context variables from our state machine.
  • Added a common config for state machines. For now it just controls the devTools option, which should be true in development.
  • Updated xstate/react to fix an issue with typings on useMachine

@codecov-commenter
Copy link

codecov-commenter commented Sep 30, 2020

Codecov Report

Merging #99 into single-task-execution will increase coverage by 0.22%.
The diff coverage is 92.38%.

Impacted file tree graph

@@                    Coverage Diff                    @@
##           single-task-execution      #99      +/-   ##
=========================================================
+ Coverage                  67.49%   67.71%   +0.22%     
=========================================================
  Files                        372      374       +2     
  Lines                       6030     6078      +48     
  Branches                     945      947       +2     
=========================================================
+ Hits                        4070     4116      +46     
- Misses                      1960     1962       +2     
Impacted Files Coverage Δ
src/components/Launch/LaunchWorkflowForm/types.ts 100.00% <ø> (ø)
...ponents/Launch/LaunchWorkflowForm/launchMachine.ts 75.51% <75.51%> (ø)
...h/LaunchWorkflowForm/useLaunchWorkflowFormState.ts 96.03% <96.29%> (+0.86%) ⬆️
...s/Launch/LaunchWorkflowForm/LaunchWorkflowForm.tsx 100.00% <100.00%> (ø)
...unchWorkflowForm/useWorkflowSourceSelectorState.ts 100.00% <100.00%> (ø)
src/components/common/constants.ts 100.00% <100.00%> (ø)
src/components/hooks/useFetchableData.ts 92.85% <100.00%> (ø)
src/components/hooks/useQueryState.ts 100.00% <100.00%> (ø)
src/components/hooks/useWorkflow.ts 42.85% <0.00%> (-57.15%) ⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 38eab6d...16861d0. Read the comment docs.

Copy link
Contributor

@BobNisco BobNisco 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 the approach of leveraging xstate more heavily!

export const launchMachine = Machine(launchMachineConfig, {
actions: {
setWorkflowVersion: assign((_, event) => ({
workflowVersion: (event as SelectWorkflowVersionEvent).workflowId
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way we can cut back on the use of casting? I was looking at the docs for xstate and I understand they don't have 100% perfect typing throughout, but I was curious if there was anyway to use type narrowing on the event passed in here or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suspected you might bring this up :-).

I tried several approaches. I think the only real way to get solid type support is to use inline functions in the machine config. That didn't fully solve the problem and it's specifically recommend against by the documentation. So my compromise was to at least narrow things down by individually defining the events I was casting to so that in the context of the action we know what the shape of the data payload should look like.

I had a similar problem with the services below here. I could not find a way with the current type definitions to make the return types of the services strongly types. At best, they will be Promise<any>. That means that the implementations that I provide when I interpret this machine in my react hook have no way of being checked that they are returning a value with the correct shape, property names, etc.

The good news is that when the actions and services don't do what is expected, the tests will blow up. I spent a fair chunk of time getting tests working again, and some of that was finding places where actions/services were returning or expecting the wrong type. So I think for now, tests and runtime checking will be the best we can do.

I have two additional enhancements in mind for a follow-up PR:

  1. Look into model-based testing for the machine. This should ensure that all machine states are reachable, that the rendered components look correct when we reach each state, and should fully exercise all of the actions/services. That should be a good signal for when things change and our typing isn't able to warn us at compile time.
  2. Some people in the community have proposed workarounds involving runtime checks that are type guards. Basically, that we should be able to test the payload sent to the action and so a is SelectWorkflowVersionEvent. That won't help catch errors at compile-time though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I really am that predictable that I'd bring up types in a PR 😆

Sounds good, I certainly have spent less time with xstate than you have, so thanks for the thoughtful comments and I'm glad we have validation here in the form of tests!

package.json Show resolved Hide resolved
@schottra schottra merged commit c5462f6 into single-task-execution Oct 2, 2020
@schottra schottra deleted the launch-tasks branch October 2, 2020 15:13
schottra added a commit that referenced this pull request Oct 8, 2020
* refactor: make workflow details generic so it can be used for tasks (#96)

* refactor: make workflow details generic so it can be used for tasks

* chore: cleanup and moving tests over

* feat: adding route and navigation to the task details page (#97)

* Refactor Launch form to use a state machine (#99)

* refactor: filling out details of the state machine for launch

* refactor: checkpoint

* refactor: mostly finished wiring of machine to component state

* refactor: more work to get form component migrated to using machine

* refactor: cleaning up state for selectors

* fix: type error due to patch version upgrade

* refactor: trying a flat state structure

* fix: getting all tests passing again

* chore: cleanup and docs

* chore: pull request feedback

* refactor: Make launch state and components generic (#100)

* refactor: splitting launch machine into two separate types

* refactor: move shared state out to component

* refactor: use composition to create workflow form

* refactor: update usage of LaunchWorkflowForm -> LaunchForm

* chore: cleanup and fix tests

* feat: Add Task support to Launch form (#101)

* feat: add task support in launch components

* test: updating launch form tests to handle task cases

* fix: remaining broken tests

* Cleanup work for launching single task executions. (#102)

* feat: add support for launch tasks in entity details view

* fix: correctly map initial parameters when relaunching

* fix: correct parent name and back link in execution details page

* fix: pass through referenceExecution when relaunching

* test: check rendering of referenceExecution

* test: adding tests for relaunch flow
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.

3 participants