primitives icon indicating copy to clipboard operation
primitives copied to clipboard

Prop types for children of slottable (`asChild`) components

Open DaniGuardiola opened this issue 3 years ago • 6 comments

Feature request

Overview

Public types representing the props that a slottable component will pass to its child when asChild={true}.

Examples in other libraries

N/A

Who does this impact? Who is this for?

Any user who wants to have better type-safety or that has to deal with non HTML-compatible components (different API than Radix is expecting for a child).

Additional context

My real-world example of this is the following:

  • We have a button component that is built with the react-aria usePress hook.
  • That hook replaces all pointer events (e.g. onMouseDown, onPointerUp, etc) with a custom, non-standard "press API" (e.g. onPressDown, onPress, etc).
  • We need to use that button as trigger for a dropdown menu (using asChild).
  • Radix passes onPointerDown, which doesn't exist on our button component, and therefore it gets ignored.

This was a tough one to debug, because we were not getting any type errors. With this type, we could do something like this:

// Radix type
export type RadixTriggerChildProps = {
  onPointerDown: (event: PointerEvent) => void,
  id: string,
  aria-whatever: string,
  // etc...
}

// user button
function CustomButton(props: ReactAriaProps) {
  const { buttonProps } = useButton(stuff)
  // etc...
}

// user trigger (erroring)
function CustomTrigger(props: TriggerChildProps) {
  return <CustomButton {...props} /> // ERROR!!!!1 (CustomButton doesn't accept onPointerDown)
}

// user trigger (correct)
function CustomTrigger({ onPointerDown: _, ...props }: TriggerChildProps) {
  // custom handling of incompatible props
  // probably by using controlled state to open the menu
  return <CustomButton {...props} onPress={() => setOpen(true)} />
}

// user dropdown menu
<>
  <RadixTrigger asChild>
    <CustomTrigger />
  </RadixTrigger>
</>

This improves quite a bit our current situation :sweat_smile: .

The specifics on how to implement the missing functionality are a different story, and that's something we want to get right to avoid hurting a11y and DX. I've got a few ideas, and I might come up with an additional (very minor) feature request that would enable a nice pattern. But that's for another day.

DaniGuardiola avatar Oct 07 '21 17:10 DaniGuardiola

this discussion got me thinking that radix could expose an AsRenderProp utility (instead of expecting consumers to create one), which got me thinking further...

perhaps asChild could accept asChild={AsRenderProp} and then use some of this thinking to type the render-prop function's props?

jjenzz avatar Apr 18 '22 01:04 jjenzz

Edit: my bad, I got some things mixed up here. Did some edits to make it make sense (or more sense at least lol).

@jjenzz I think if children is typed like this:

// whatever is passed to the user-provided component
type PassedProps = { onClick: (e) => someBuiltingBehaviorIdk(e) }
type Props = {
  children: (props: PassedProps) => ReactNode | ReactNode
}

...then the function member of the union should kick in whenever you pass a render function as child, with strongly typed props as argument. Radix could then just handle the render child. Ariakit does something like this and it works well in my experience.

Isn't that better than having to specify the props yourself?

DaniGuardiola avatar Apr 19 '22 16:04 DaniGuardiola

AsRenderProp is better than nothing, but it kinda seems like render function child with extra steps :P

DaniGuardiola avatar Apr 19 '22 16:04 DaniGuardiola

thanks @DaniGuardiola, i do already know how render-props work tho, apologies for the confusion :)

the proposal is because radix team has historically been against having each component support multiple apis. it would mean that only AsRenderProp would need tests instead of testing every component to validate a render-prop api exists and is implemented properly.

components can stay as-is and consumers can opt-in to render-prop behaviour case-by-case.

tbh this is maybe one step too far/unnecessary:

perhaps asChild could accept asChild={AsRenderProp} and then use some of this thinking to type the render-prop function's props?

if each component exposed the types like originally requested, radix could expose an AsRenderProp utiliity and then you have all the bits you need to do what i did in the original sandbox.

i wish TS just supported typing children: React.ReactElement<{ props: '', here: ''}> like flow does tbh, then this would all go away.

jjenzz avatar Apr 20 '22 12:04 jjenzz

@jjenzz sorry if it sounded like I was trying to explain how they work to you, 100% not my intention :sweat_smile: I was rather aiming to explain my thinking and make my point. Honestly I've learned a lot from reading your code/blog and I have a lot of respect for you, I don't think I could teach you much even if I wanted to anyway lol.

I understand about the Radix predisposition about multiple APIs. AsRenderProp is definitely an intriguing idea, although what I would really love is to find a way to make the props have the correct types without delegating that responsibility to the user, but react children types are weak as you know so not sure that's possible without using a function (kinda what I was trying to illustrate in my comment). Agreed that the ideal scenario would be typescript/react supporting better children types. Didn't know that flow had better support, that's interesting.

I haven't been able to look too closely at your sandboxes/linked resources so let me know if I'm missing or misunderstanding something!

DaniGuardiola avatar Apr 20 '22 16:04 DaniGuardiola

ah no hard feelings at all, i just realised my message may have confused things as it does come across as a roundabout approach.

although what I would really love is to find a way to make the props have the correct types without delegating that responsibility to the user

that is what my asChild={AsRenderProp} suggestion was aiming to do, type the props without the need for each of the components to provide a render-prop api. the idea was this:

<Tooltip.Trigger asChild={AsRenderProp}>
  {(props, ref) => ( // these params would be typed
    <Button />
  )}
</Tooltip.Trigger>

jjenzz avatar Apr 20 '22 19:04 jjenzz