[ChoiceList] Improve the type safety and correctness of ChoiceList

[jas7457]

WHY are these changes introduced?

The TypeScript types for ChoiceList weren't quite right. It says that selected, choices.value, and onChange were simple strings. This works for a lot of cases, but using either an enum or string literals, this breaks down a bit. Here's a simple example:

function Test() {
  const [selected, setSelected] = useState<
    ('hidden' | 'optional' | 'required')[]
  >(['hidden']);

  return (
    <ChoiceList
      title="Company name"
      choices={[
        {label: 'Hidden', value: 'hidden'},
        {label: 'Optional', value: 'optional'},
        {label: 'Required', value: 'required'},
      ]}
      selected={selected}
      onChange={setSelected}
    />
  );
}

WHAT is this pull request doing?

This PR improves the TypeScript types to use generics. The type still needs to extend a string, but now, we can know exactly what type of string it is. The type will be inferred by selected and will propagate to the choices.value and onChange handlers.

Default case: not supplying any types

In this case, `selected` is inferred as `string[]` so it should effectively behave as before.

useState generic case

In this case, supplying the real type of `selected` will allow `onChange` to be typed properly

This also has the benefit that you cannot supply any choice values that are not allowed

useState type-casting case

In this case, as with the above, TS will supply the proper type to `onChange`

Similar to above, this also has the benefit that you cannot supply any choice values that are not allowed

Caveat

There's one caveat to this change that is worth pointing out. If the user has correctly typed their useState to something besides string[] AND they are not defining their choices inline, TS will type-widen their value's type to string and will cause a TS issue. They will need to type the choices to fix this. For example:

type HiddenOptionalRequired = 'hidden' | 'optional' | 'required';

export function Default() {
  const [selected, setSelected] = useState<HiddenOptionalRequired[]>([
    'hidden',
  ]);

  const choices: ChoiceListProps<HiddenOptionalRequired>['choices'] = [
    {label: 'Hidden', value: 'hidden'},
    {label: 'Optional', value: 'optional'},
    {label: 'Required', value: 'required'},
  ];

  return (
    <ChoiceList
      title="Company name"
      choices={choices}
      selected={selected}
      onChange={setSelected}
    />
  );
}

🎩 checklist

• Tested on mobile
• Tested on multiple browsers
• Tested for accessibility
• Updated the component's README.md with documentation changes
• Tophatted documentation changes in the style guide

[kyledurand]

/snapit

[kyledurand]

Can you use the snapshot to make sure CI passes in web?

[github-actions]

🫰✨ Thanks @kyledurand! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @shopify/polaris-icons@0.0.0-snapshot-release-20231114175900

yarn add @shopify/polaris-migrator@0.0.0-snapshot-release-20231114175900

yarn add @shopify/polaris@0.0.0-snapshot-release-20231114175900

yarn add @shopify/polaris-tokens@0.0.0-snapshot-release-20231114175900

yarn add @shopify/stylelint-polaris@0.0.0-snapshot-release-20231114175900

[jas7457]

/snapit

[github-actions]

🫰✨ Thanks @jas7457! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @shopify/polaris-icons@0.0.0-snapshot-release-20231114205947

yarn add @shopify/polaris-migrator@0.0.0-snapshot-release-20231114205947

yarn add @shopify/polaris@0.0.0-snapshot-release-20231114205947

yarn add @shopify/polaris-tokens@0.0.0-snapshot-release-20231114205947

yarn add @shopify/stylelint-polaris@0.0.0-snapshot-release-20231114205947

[jas7457]

@kyledurand I just tested pulling it into web here and have 1 type error. This seems to have been caused by this PR and nothing to do with my change.

[aaronccasanova]

Nice update! Type changes look good from my initial pass. That said, I do wonder if this update could break builds for existing implementations (in and outside the org).

There's one caveat to this change that is worth pointing out. If the user has correctly typed their useState to something besides string[] AND they are not defining their choices inline, TS will type-widen their value's type to string and will cause a TS issue. They will need to type the choices to fix this. For example:

Would as const be sufficient?

type HiddenOptionalRequired = 'hidden' | 'optional' | 'required';

export function Default() {
  const [selected, setSelected] = useState<HiddenOptionalRequired[]>([
    'hidden',
  ]);

  const choices = [
    {label: 'Hidden', value: 'hidden'},
    {label: 'Optional', value: 'optional'},
    {label: 'Required', value: 'required'},
  ] as const;

  return (
    <ChoiceList
      title="Company name"
      choices={choices}
      selected={selected}
      onChange={setSelected}
    />
  );
}

[jas7457]

Would as const be sufficient?

Unfortunately, no. This makes choices a ReadOnly array, not just an array. Originally, I allowed for choices to be readonly so that as const would work, but this ran into a problem in web as they had some choices and then conditionally pushed to that array (which isn't allowed on readonly).

Because of that, I got rid of allowing readonly in this commit.

That said, I do wonder if this update could break builds for existing implementations (in and outside the org).

Potentially, if they were correctly typing their selected but not their choices (it will work if inlined, but not if they don't inline it and let TS infer the type to just string), like in the caveat I mention. Due to that, do you consider this a breaking change? I'm not sure what the "correct" semver is for something like this.

[aaronccasanova]

Thanks for clarifying! That all makes sense to me 👍

Due to that, do you consider this a breaking change? I'm not sure what the "correct" semver is for something like this.

A little, yes. Other packages in Polaris (such as Stylelint Polaris) apply major versions when an upgrade has the potential to break builds. That said, I'm ensure how strictly that is followed in Polaris React and would be interested to hear thoughts from @sam-b-rose or @alex-page

[sam-b-rose]

If if could be backward compatible and support the narrower type improvement and the original functionality, that would be a minor.

Since this does affect consumers and potentially break builds due to TS changes, I would consider it a major as well.

I really like this improvement in typing! We can either keep it around for the next major or try to working in support for both. I am partial to the latter so we can ship it sooner.

[jas7457]

We can either keep it around for the next major or try to working in support for both.

@sam-b-rose did you have a way in mind to support both? I'm not sure what that would look like, exactly.

Since this does affect consumers and potentially break builds due to TS changes, I would consider it a major as well.

If we do in fact need to consider it major, which I'm fine with, I think we could make a similar improvement to a bunch of different components. OptionList, IndexTable, Autocomplete, Listbox, etc come to mind.

[aaronccasanova]

We can either keep it around for the next major or try to working in support for both.

@sam-b-rose did you have a way in mind to support both? I'm not sure what that would look like, exactly.

I created a simplified playground and wasn't able to quickly find backward compatible typings. Also, curious if you have any ideas @sam-b-rose

[translation-platform]

Localization quality issues found

The following issues may affect the quality of localized translations if they are not addressed:

• The value Clear for key Polaris.ActionList.SearchField.clearButtonLabel is very short. Short strings are more likely to be misunderstood by translators without context. Please provide additional context for the translators if possible.
• The value Search for key Polaris.ActionList.SearchField.search is very short. Short strings are more likely to be misunderstood by translators without context. Please provide additional context for the translators if possible.
• The value Search actions for key Polaris.ActionList.SearchField.placeholder is very short. Short strings are more likely to be misunderstood by translators without context. Please provide additional context for the translators if possible.

Please look out for other instances of this issue in your PR and fix them as well if possible.

Questions about these messages? Hop in the #help-localization Slack channel.

[github-actions]

Hi! We noticed there hasn’t been activity on this PR in a while. After 30 days, it will close automatically.

If it’s still relevant, or you have updates, comment and let us know. And don’t worry, you can always re-open later if needed.