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

Handle validation of list inputs #31

Merged
merged 6 commits into from
Dec 20, 2019
Merged

Conversation

schottra
Copy link
Contributor

@schottra schottra commented Dec 20, 2019

flyteorg/flyte#22

There will be a follow-up PR for unit tests. This is against a feature branch, so it doesn't need to be 100% complete to merge

  • Changed inputConverters to inputHelpers and moved them into a separate folder with a file for each type. Input helpers export functionality for validating values as well as converting them to ILiterals for submission.
  • Separated out the rendering of inputs in the form into a different component.
  • Moved input state management out of useLaunchWorkflowFormState into a separate hook. The form state hook will communicate with the input state hook (to get values when submitting) via a ref.
  • Added explicit validation before submitting.
  • Updated input components to handle rendering errors if they exist
  • Added functionality to the form/state to only show errors after the first submission
  • Fixed an issue with useFetchable not fully resetting state when its cache key changes. This was causing any submission errors to persist after changing the workflow/launch plan.
  • getCacheKey can now also accept an array

Note
The input rendering was moved into a separate component so that I could take advantage of component lifecycle to do something a little strange.
The logic for managing input values and validation gets a lot easier if each input is managed by its own nested state hook. However, the list of inputs is dynamic and changes every time the workflow/launch plan combination changes. This means that something like inputs.map(input => useInputState(input)) will cause problems if the value of inputs changes at all within the same instance of a component.
The solution I landed on was to generate a key value that's based on hashing the array of inputs, and use that with the component that renders the inputs. This has the effect of creating a new instance of the component any time the list of inputs changes. And it means that the map we do will be guaranteed to run on the same list of inputs, in the same order, and result in the same ordering of hooks calls.

ListValidation

@schottra schottra requested a review from mrmcduff December 20, 2019 21:09
}

/** Manages the state (value, error, validation) for a list of `ParsedInput` values.
* NOTE: The input value for this hook is used to generate sub-hooks.

Choose a reason for hiding this comment

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

Good to have this warning here -- I was trying to think of a way to enforce it on the code side, but you probably don't want any reference to the mount state of the component

@schottra schottra merged commit 363864f into launch-enhancements Dec 20, 2019
@schottra schottra mentioned this pull request Jan 8, 2020
@schottra schottra deleted the launch-errors branch February 5, 2020 19:32
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