Terra-core: [terra-form-select] Array of numbers for MultiSelect produces proptype warnings

[PseudoFaux]

Bug Report

Description

Previously on terra-form-select 5.25.0, given a multiple variant (MultiSelect or TagSelect), we could pass a list of numbers for the value prop:

Select.jsx:100
SelectField.jsx:102

<Select
  variant={'multiple'}
  value={[12, 34, 56, 78]}
>
  {...}
</Select>

When the large refactor happened for terra-form-select 5.26.0, the proptype validation requires value to be an array of strings. MultiSelect.jsx:93

Steps to Reproduce

Create a Select component with a 'multiple' variant
Set the value to an array of numbers
Receive proptype validation warnings

Additional Context / Screenshots

Expected Behavior

This code would not produce a proptype error.

Possible Solution

Update MultiSelect.jsx:93 to accept an array of numbers or strings

Environment

• Component Name and Version: [email protected]+
• Browser Name and Version: Latest Chrome
• Node/npm Version: Node 8, npm 5
• Operating System and version (desktop or mobile): Any

@ Mentions

@dev-cprice

[StephenEsser]

Adding a discussion from the Terra support channel with a similar issue.

Hi All, I’m facing issue while consuming terra-form-select 5.27 version. Some validation warnings are coming up in the console, same error screenshot I’ve attached.

I went through the terra documentation where I figured out that Select tag accepts value as an array, string or number and SearchSelect accepts value as string or number only. It looks mismatch in select and SearchSelect value acceptance.

So from our workflow we are sending value as an array to Select and Select sends that array further to SearchSelect which is creating validation error in SearchSelect I think.

The Select previously allowed passing an array for any variant type and had logic to extract the data as necessary for each variant. I do think it is odd / inappropriate to pass an array into a variant type that only allows a single selection, but from a passivity perspective this no longer works.

[nramamurth]

@StephenEsser

I do think it is odd / inappropriate to pass an array into a variant type that only allows a single selection, but from a passivity perspective this no longer works.

I totally agree. However, does restoring passivity mean that the original bug be reintroduced?

I don't think that it makes sense for single select variants to accept an array for 'selected value', tho. I am not a big fan but one thing that I can think of is to maybe add a custom prop validator in Select to up-front validate the value prop based on the variant and keep the rest of the current code as-is. That is nothing different from the current behavior except for the warning will be seen in Select and not a level below. In addition to that, I'd lean towards saying that the changes that went into v5.25.0 have corrected an existing flaky behavior and is _not_ a breaking change.

[StephenEsser]

This was not a bug prior to the changes made in v5.25.0. The Select had a consistent PropType expectation for all variants.

This worked both ways, the multiple Select variant could also accept a string as a defaultValue.

This avoided a complex prop driven custom PropType validator and allowed consistent behavior and expectations between all variants.

As the Select is being broken out into individual resources I do think the more strict PropTypes are appropriate. I do not think the <SearchSelect /> should accept an array. The issue is being passive with the original <Select /> API.

[nramamurth]

I do not think the <SearchSelect /> should accept an array. The issue is being passive with the original <Select /> API.

Right. In that case, how do we handle it if the consumers today have been sending down an array for variants that support _only_ single selection in addition to satisfying the condition that those variants should not accept an array? Do we have the liberty to perhaps internally flatten the array or extract the first or the last entry from the array? I don't think that solves the issue but I am just putting it out there. I am trying to think out loud how the outcome can be passive by satisfying both the conditions.

[dev-cprice]

Well, the original code just did a find if value was a list. That's all we need to do again

[pranav300]

@StephenEsser and @dev-cprice, I am sorry the above comments are a bit confusing to me.

Just that we are all on the same page, do we want to keep the current Search API passive (meaning that the <Select /> accepts number/array/string for the value prop) and also make sure that Single Selection variants don't accept an array and don't throw up when they do? If yes, then I am not sure about how to satisfy both the constraints together.

If not, then does it mean that we will let Single Selection variants to accept an array (like they previously did) to keep the functionality passive with what they are doing today? Maybe it is okay to introduce a custom prop validator for <Select /> when we do MVB in the future?

[bjankord]

My preference is let's allow the previous behavior (let Single Selection variants to accept an array) to keep the functionality passive. When we do the next MVB for this package, we can reconsider how we want that to work.

[dev-cprice]

I can get a PR open for this real quick

[dev-cprice]

PR to fix prop-type passivity for all variants: https://github.com/cerner/terra-core/pull/2646