Closed acelaya closed 1 month ago
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 100.00%. Comparing base (
e2e88a3
) to head (12d8e14
).
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
This PR exposes two new aliases for
SelectNext
, which areSelect
andMultiSelect
. These two implicitly havemultiple={false}
andmultiple={true}
respectively, and do not allow themultiple
prop to be provided.This change serves two purposes:
onChange
's argument type to no longer be inferred based on the type ofvalue
.Select
andMultiSelect
are now enforcing one of the two type paths whenmutiple
is true or false, fixing the type inference. For more context on the issue, check this comment and the ones below.Select
can transparently replace existingSelectNext
usages, allowing us to eventually drop theSelectNext
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.
TODO
Select
andMultiSelect
in client and LMS projects, where<SelectNext />
and<SelectNext multiple />
is used, to confirm they work.SelectNext
withSelect
. 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 eitherSelect
orMultiSelect
, as they will provide the same coverage, and promote their usage overSelectNext
.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.