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

Add support for Collections in the launch UI #30

Merged
merged 6 commits into from
Dec 18, 2019

Conversation

schottra
Copy link
Contributor

flyteorg/flyte#22

(This is being opened against a feature branch. I would like to add some more handling of errors and mapping to the fields in the form before merging this to master)

Collections are a list of Literal values which all have the same type. The inner type of a Collection may be another Collection, making nesting possible.

Our initial support for collections will require users to enter values as a valid string of JSON, which we will parse into native JS(ON) types (string, number, boolean) and then convert to the appropriate Literal type based on the type of the collection. It's a little clunky, but easy to validate and uses a well-known standard.

Some important points:

  • We're using lossless-json for the parsing because it preserves integers that would otherwise overflow and gives us the ability to directly convert them to Longs (which are used in our protobuf message classes to handle int64s).
  • For boolean, we will allow a handful of truthy/falsey values for convenience (0, 1, '0', '1', true, false, 'true', 'false', 't', 'f')
  • Because it's a JSON string, the user must add their own brackets. We could try and do this for them, but since collections can contain nested collections, trying to handle the edge cases of how many brackets to add doesn't seem worth the complication.
  • Also because it's JSON, string values must be in double quotes (""). This differs from entering single string values, which do not require quotes.

I also made some changes to the input conversion functions:

  • The input now contains the type information (collections need this to determine the nested converter to call).
  • The input value is now an InputValue instead of being explicitly toString-ed beforehand. This puts the responsibility on the conversion function for converting to a string if necessary
  • Most of the input converters can now accept the native type in addition to a string value. This was needed to support the collection parsing case.

And I also added a lot of test cases to cover all of the expected variations for values to be parsed, collections of those value types, and singly-nested collections of those value types.

@schottra schottra requested a review from mrmcduff December 18, 2019 19:54
Copy link

@mrmcduff mrmcduff left a comment

Choose a reason for hiding this comment

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

Looks good -- only had a few questions, possibly because I've never used the lossless-json library before.

fullWidth={true}
label={label}
multiline={true}
onChange={stringChangeHandler(onChange)}

Choose a reason for hiding this comment

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

Where does the input value to stringChangeHandler here get defined? Is it recursively referencing the onChange handler of the TextField? So, x = stringChangeHandler(x)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

stringChangeHandler generates an event handler that will forward the value received to its argument. So in this case, the prop value ends up looking like:

onChange = ({ target: { value } }) => props.onChange(value); // props here are the props to CollectionInput

function booleanToLiteral(value: string): Core.ILiteral {
return { scalar: { primitive: { boolean: Boolean(value) } } };
function losslessReviver(key: string, value: any) {
if (value && value.isLosslessNumber) {

Choose a reason for hiding this comment

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

Does this handle a value of 0?

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 see why you're asking (0 is falsey). Your intuition is correct, but it should be fine in this case, as the behavior is just to return value directly. That would result in a number and is the desired behavior.

@schottra schottra merged commit ad6a545 into launch-enhancements Dec 18, 2019
@schottra schottra deleted the more-input-types branch December 18, 2019 22:55
@schottra schottra mentioned this pull request Jan 8, 2020
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