Expose Select and MultiSelect aliases for SelectNext #1619

acelaya · 2024-07-16T14:41:32Z

This PR exposes two new aliases for SelectNext, which are Select and MultiSelect. These two implicitly have multiple={false} and multiple={true} respectively, and do not allow the multiple prop to be provided.

This change serves two purposes:

The main one is to solve an issue introduced in Allow multiple selection in SelectNext #1601, which causes onChange's argument type to no longer be inferred based on the type of value.
Select and MultiSelect are now enforcing one of the two type paths when mutiple is true or false, fixing the type inference.
For more context on the issue, check this comment and the ones below.
Additionally, it creates new simpler component names, where Select can transparently replace existing SelectNext usages, allowing us to eventually drop the SelectNext symbol in the next major release.

I tried to find a solution to the type inference at pure types definition level, but didn't quite get it, even after making types more complex, so I think this option is better, specially taking into consideration to additional benefit explained above.

Note

This PR is easier to review ignoring whitespaces.

TODO

Document new component aliases and use them in examples to prove they work.
Test Select and MultiSelect in client and LMS projects, where <SelectNext /> and <SelectNext multiple /> is used, to confirm they work.
- EDIT: Exposed these changes via yalc, and imported them in client and LMS, then replaced all SelectNext with Select. Everything works after a manual test, tests pass with a few minor changes, and typechecks pass.

Out of scope

We can probably replace all usages in this library of SelectNext with either Select or MultiSelect, as they will provide the same coverage, and promote their usage over SelectNext.

Additionally, we could even mark SelectNext as deprecated as a standalone component, and recommend using one of the other two.

All that will come in follow-up PRs, to avoid distractions from the new API being introduced here.

acelaya · 2024-07-16T14:45:35Z

src/components/input/SelectNext.tsx


-export default SelectNext;
+export const MultiSelect = Object.assign(


I chose MultiSelect over SelectMulti for consistency with SingleValueProps and MultiValueProps, but the latter allows for a nicer grouping of symbols when sorted alphabetically.

acelaya · 2024-07-17T06:43:41Z

src/components/input/SelectNext.tsx

+   * and the value must be an array.
+   * Defaults to false.
+   */
+  multiple?: boolean;


I considered moving this to SingleValueProps and MultiValueProps, but that breaks other things, so I decided to leave it as is.

With Select and MultiSelect, this prop's value will be implicit and consumers will not have to provide it, so it's not too bad.

codecov · 2024-07-17T06:46:17Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (e2e88a3) to head (12d8e14).

Additional details and impacted files

@@            Coverage Diff            @@
##              main     #1619   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           62        62           
  Lines         1066      1071    +5     
  Branches       410       410           
=========================================
+ Hits          1066      1071    +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

acelaya · 2024-07-17T06:51:48Z

src/components/input/test/SelectNext-test.js

@@ -21,20 +21,26 @@ describe('SelectNext', () => {
   *        Whether to renders SelectNext.Option children with callback notation.
   *        Used primarily to test and cover both branches.
   *        Defaults to true.
+   * @param {MultiSelect | Select | SelectNext} [options.Component] -


I'm not completely sure this JSDoc is correct

The JSDoc here is only for information and not typed checked, so this is fine as long as the meaning is clear.

acelaya · 2024-07-17T06:53:26Z

src/pattern-library/components/patterns/prototype/SelectNextPage.tsx

+          <code>SelectNext</code> (and its aliases <code>Select</code> and{' '}
+          <code>MultiSelect</code>) are composite components which behave like
          the native <code>{'<select>'}</code> element.
        </p>
      }
    >
      <Library.Section>
        <Library.Pattern>
-          <Library.Usage componentName="SelectNext" />
+          <Library.Usage componentName="MultiSelect, Select, SelectNext" />


acelaya · 2024-07-17T06:55:01Z

src/pattern-library/examples/select-next-basic.tsx

        value={value}
-        onChange={setSelected}
+        onChange={newValue => setSelected(newValue)}


I used an inline callback here just to demonstrate type is properly inferred with Select, as this file is type-checked.

robertknight · 2024-07-17T08:08:43Z

src/components/input/SelectNext.tsx

-  };
+export type MultiSelectProps<T> = BaseSelectProps & MultiValueProps<T>;
+
+export type SelectNextProps<T> = (SelectProps<T> | MultiSelectProps<T>) & {


Other than being a breaking change, are there any reasons to keep SelectNext around? Can we replace all uses with either Select or MultiSelect?

Nop, just the breaking change. I already have ~~PRs~~ branches in both client and LMS replacing all SelectNext instances and they all transparently work.

robertknight

The main feedback I have is that we should make a major version update soon that removes SelectNext from the public API and revises the documentation to make the preferred use of the new Select/MultiSelect components obvious. In its current state, I think the documentation will be confusing to someone who hasn't been following the discussions about the evolution here.

src/components/input/SelectNext.tsx

robertknight · 2024-07-17T09:41:28Z

src/components/input/test/SelectNext-test.js

@@ -21,20 +21,26 @@ describe('SelectNext', () => {
   *        Whether to renders SelectNext.Option children with callback notation.
   *        Used primarily to test and cover both branches.
   *        Defaults to true.
+   * @param {MultiSelect | Select | SelectNext} [options.Component] -


The JSDoc here is only for information and not typed checked, so this is fine as long as the meaning is clear.

acelaya commented Jul 16, 2024

View reviewed changes

acelaya force-pushed the type-safe-select branch from cf8da0f to 5546abe Compare July 17, 2024 06:41

acelaya marked this pull request as ready for review July 17, 2024 06:41

acelaya force-pushed the type-safe-select branch from 5546abe to d20abac Compare July 17, 2024 06:43

acelaya commented Jul 17, 2024

View reviewed changes

acelaya requested a review from robertknight July 17, 2024 06:44

acelaya commented Jul 17, 2024

View reviewed changes

acelaya force-pushed the type-safe-select branch from d20abac to 6495ccc Compare July 17, 2024 06:54

acelaya commented Jul 17, 2024

View reviewed changes

acelaya force-pushed the type-safe-select branch from 6495ccc to f13d045 Compare July 17, 2024 06:55

acelaya mentioned this pull request Jul 17, 2024

Update to latest frontend-shared with SelectNext UI enhancements hypothesis/lms#6449

Closed

robertknight reviewed Jul 17, 2024

View reviewed changes

robertknight approved these changes Jul 17, 2024

View reviewed changes

Expose Select and MultiSelect aliases for SelectNext

12d8e14

acelaya force-pushed the type-safe-select branch from f13d045 to 12d8e14 Compare July 17, 2024 10:12

acelaya merged commit 73dfdf2 into main Jul 17, 2024
4 checks passed

acelaya deleted the type-safe-select branch July 17, 2024 10:16

acelaya mentioned this pull request Jul 17, 2024

Refactor Select documentation to no longer mention SelectNext #1620

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose Select and MultiSelect aliases for SelectNext #1619

Expose Select and MultiSelect aliases for SelectNext #1619

acelaya commented Jul 16, 2024 •

edited

Loading

acelaya Jul 16, 2024 •

edited

Loading

acelaya Jul 17, 2024

codecov bot commented Jul 17, 2024 •

edited

Loading

acelaya Jul 17, 2024

robertknight Jul 17, 2024

acelaya Jul 17, 2024

acelaya Jul 17, 2024

robertknight Jul 17, 2024

acelaya Jul 17, 2024 •

edited

Loading

robertknight left a comment

robertknight Jul 17, 2024


		export default SelectNext;
		export const MultiSelect = Object.assign(

Expose Select and MultiSelect aliases for SelectNext #1619

Expose Select and MultiSelect aliases for SelectNext #1619

Conversation

acelaya commented Jul 16, 2024 • edited Loading

TODO

Out of scope

acelaya Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

acelaya Jul 17, 2024

Choose a reason for hiding this comment

codecov bot commented Jul 17, 2024 • edited Loading

Codecov Report

acelaya Jul 17, 2024

Choose a reason for hiding this comment

robertknight Jul 17, 2024

Choose a reason for hiding this comment

acelaya Jul 17, 2024

Choose a reason for hiding this comment

acelaya Jul 17, 2024

Choose a reason for hiding this comment

robertknight Jul 17, 2024

Choose a reason for hiding this comment

acelaya Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

robertknight left a comment

Choose a reason for hiding this comment

robertknight Jul 17, 2024

Choose a reason for hiding this comment

acelaya commented Jul 16, 2024 •

edited

Loading

acelaya Jul 16, 2024 •

edited

Loading

codecov bot commented Jul 17, 2024 •

edited

Loading

acelaya Jul 17, 2024 •

edited

Loading