mui / base-ui

Base UI is an open-source library of accessible, unstyled UI components for React.
MIT License
291 stars 47 forks source link

[POC] asChild #801

Closed michaldudak closed 2 weeks ago

michaldudak commented 3 weeks ago

asChild API added to all components. It lives alongside the render prop (to minimize the number of changes), but takes precedence over it.

Live demo: https://deploy-preview-801--base-ui.netlify.app/experiments/asChild

mui-bot commented 3 weeks ago

Netlify deploy preview

https://deploy-preview-801--base-ui.netlify.app/

Generated by :no_entry_sign: dangerJS against b4a091657446822c20b37a532b911690641d6d33

vladmoroz commented 3 weeks ago

@michaldudak I took a quick look, here's a few things I noticed:

(1) If you create a component that wraps a Base UI part and destructure children from props there, TS doesn't let you have it:

image image

(2) Obviously, I can't use interfaces since the type isn't static anymore:

interface MyDialogTitleProps extends React.ComponentProps<typeof Dialog.Title> {
  ...
}

My understanding is that TS performs much better with interfaces and static types.


(3) Does function as children need to be contingent on asChild={true}? Arguably, it's reasonable to use a render function without replacing the original tag.


Altogether, how would you feel about doing a static type for children, and use runtime checks to check for incompatible values? It's nice that I get a type error when using asChild I pass something other than a React.ReactElement as children, but I'm really worried about TS issues like (1) and the potential TS performance hit.

Radix Primitives don't do anything fancy with the children type and it works fine. In Radix Themes I had added this runtime helper though to get nicer errors when a non-aschildable element was used.

michaldudak commented 3 weeks ago

My understanding is that TS performs much better with interfaces and static types.

AFAIK it used to be the case that interfaces were faster to typecheck, but not anymore. Obviously, a more complex type will be slower, so in this case, we might hit a small slowdown, but it shouldn't really be noticeable (it's not as bad as polymorphism, for instance).

Does function as children need to be contingent on asChild={true}? Arguably, it's reasonable to use a render function without replacing the original tag.

It doesn't. In some cases it makes perfect sense to have content rendered conditionally with a function. But such a function would have a different signature (likely (state) => ReactNode instead of (props, state) => ReactElement).

Altogether, how would you feel about doing a static type for children, and use runtime checks to check for incompatible values?

The shorter the feedback loop, the better the DX is. Having looser type, as in Radix, is an option, but it wouldn't be my preferred one.

Radix Primitives don't do anything fancy with the children type and it works fine.

"works fine" is subjective ;) For example, the error you get when you pass two children or a Fragment to a component with asChild isn't too helpful:

image

vladmoroz commented 3 weeks ago

The shorter the feedback loop, the better the DX is. Having looser type, as in Radix, is an option, but it wouldn't be my preferred one.

I agree it's much better in an ideal world. I just have PTSD from working in a codebase where TS server stops to a halt every 30 mins 😬

If we resolve issues like (1) it would be a great start

michaldudak commented 3 weeks ago

(1) looks like a limitation of TypeScript. When you destructure children, its type is no longer tied to the type of asChild, and you get the error.

Having something as silly as

if (asChild) {
  return (
    <BaseDialog.Title {...props} className={styles.Title}>
      {children}
    </BaseDialog.Title>
  );
} else {
  return (
    <BaseDialog.Title {...props} className={styles.Title}>
      {children}
    </BaseDialog.Title>
  );
}

works around this problem, but we can't expect our users to write such weird code.

We might improve the situation a bit with generic props (similarly to how Material UI's Autocomplete props work: https://github.com/mui/material-ui/blob/master/packages/mui-material/src/Autocomplete/Autocomplete.d.ts#L159), but it would make props of all components generic (so less readable and performant).