[toggle-group][toggle] type safety

[michaelhazan]

Closes #3142.
Closes #3756.

Note

Originally this was meant to only introduce generic types to the ToggleGroup, but i realized while working on this that Toggle too had to be changed to handle a generic value.

• I have followed (at least) the PR section of the contributing guide.

[pkg-pr-new]

• base-ui-tanstack-start

• vite-css-base-ui-example

  pnpm add https://pkg.pr.new/mui/base-ui/@base-ui/react@3173
  

  pnpm add https://pkg.pr.new/mui/base-ui/@base-ui/utils@3173
  

commit: cf9b46e

[mui-bot]

Bundle size report

Bundle	Parsed size	Gzip size
@base-ui/react	0B(0.00%)	0B(0.00%)

Details of bundle changes

---

Check out the code infra dashboard for more information about this PR.

[netlify]

✅ Deploy Preview for base-ui ready!

Name	Link
🔨 Latest commit	cf9b46e
🔍 Latest deploy log	https://app.netlify.com/projects/base-ui/deploys/69793f0a4026b7000809752d
😎 Deploy Preview	https://deploy-preview-3173--base-ui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[michaelhazan]

@mj12albert sorry it took me so long, work had me backed up 😅

it should be good now, because Toggle now extends BaseUIComponentProps, i had to make the value an extendable of number | string, should nonetheless be an improvement to use with Enums and Constant Objects :D

[LukasTy]

@michaldudak, is this still relevant after merging #3770?

[michaldudak]

@LukasTy, yes, I haven't noticed this PR. It's basically an equivalent of what I've recently done in #3788.

This approach has a small issue we discussed internally:

When Toggle's value is undefined, we generate it using useBaseUiId, which returns a string. So we can't really guarantee that ToggleGroup<T> with Toggle<T> components inside is indeed type-safe. Options to solve it could be:

• Enforce a Toggle's value when it's used within a ToggleGroup. It actually doesn't make much sense otherwise, but technically is a breaking change.
• Ignore it. Since the contract between Toggle and ToggleGroup is weak anyway (we can't enforce them to use the same type of value), it will be up to developers to make sure the correct values are provided. I think it should be possible to make it a bit more obvious by requiring value on a type level whenever a type parameter is set explicitly (so <Toggle<'left' | 'right'> would error in TS without a value)

Also, I'm afraid we can't use | number at this point, as it would break all the codebases that assume it's a string.

[LukasTy]

@michaldudak Thanks for sharing more details. 🙏
Do you think it makes sense to keep this PR open for v2 as a breaking change, depending on which route we decide to go with (enforce a value or consider supporting | number as well if there is enough need for it)?

[michaldudak]

We discussed this with the team and agreed that we can proceed with this implementation ignoring the potentially incorrect types. To further prevent surprises, we can add a dev-time warning when a Toggle within a ToggleGroup doesn't have an explicit value.

@michaelhazan, would you like to continue working on this?

[michaelhazan]

@michaelhazan, would you like to continue working on this?

Sure!
I will merge it with main and add the warning 👍

[michaelhazan]

@michaldudak Hey! should be good now..
if you have anymore suggestions you want me to implement let me know!

[LukasTy]

Thank you for your effort. 🙏
Leaving a few suggestions and one discussion point.

[LukasTy]

Correct me if I'm wrong, but is there a use case for why we still need the generic here? 🤔

As far as I can see, it only helps with type inference, like if you have the following:

type Alignment = 'horizontal' | 'vertical';

const alignments: Alignment[] = ['horizontal', 'vertical'];

<ToggleGroup
  multiple
  defaultValue={alignments}
  // this is going to know that the type is `Alignment` instead of plain string as it is now
  value={}
>

But, the current types don't seem to block the above usage as well.

If we go with this, then we should probably adjust other components (i.e., RadioGroup, CheckboxGroup) accordingly.

WDYT @michaldudak?

[mj12albert]

I think this is useful and addresses #3756

(it should also be able to infer the value arg in onValueChange I assume)

[LukasTy]

Gotcha. Makes sense, especially given we already have others noticing this shortcoming. 👍
I think we need to do a follow-up and address this issue for all relevant components.

[michaelhazan]

@LukasTy would you like me to fix it in this pr? or leave it for now?

[LukasTy]

I think we can keep this one isolated to these specific components.
WDYT?

[michaelhazan]

for sure

[michaelhazan]

@LukasTy fixed!
was uncertain if you wanted me to implement something from your comment here, from what i got, you want to make a general change in the library following what i've done here and #3756 , and therefore no change in necessary in this pr.

did i get that right? 😅

please note i've also set the props's generic type to equal string by default after i saw similar thing in SliderRootProps.
here and here

[mj12albert]

What type issue does this cause?
This seems like a clumsy but legit way to use toggle group without values:

<ToggleGroup>
  <Toggle onPressedChange={setState1} />
  <Toggle onPressedChange={setState2} />
</ToggleGroup>

[LukasTy]

I think Michal raised this point that in case the <ToggleGroup> defaultValue would have a type of Alignment[], then you'd end up in a slight type discrepancy, because internally a Toggle without a value would have generated an internal value type of string.
However, this is a very edge case condition. 🤔
Would it make more sense if the warning would be shown only in your case (no value on the Toggle and no default and no direct value on the Toggle Group)?