Consider adding children as a function to use instead of Slot

[rafgraph]

Hi, please consider adding the children-as-a-function pattern as an option to use instead of Slot for creating closed component APIs.

Example of how this would work using Tooltip:

// note that in the TooltipPrimitive.Trigger/Primitive code you can do:
// return typeof children === 'function' ? children(radixProps) : render_primitive_normally...

const MyTooltip = ({ children, content }) => (
  <TooltipPrimitive.Root>
    <TooltipPrimitive.Trigger>
        {children}
    </TooltipPrimitive.Trigger>
    <TooltipPrimitive.Content>
      {content}
      <TooltipPrimitive.Arrow />
    </TooltipPrimitive.Content>
  </TooltipPrimitive.Root>
);

<MyTooltip content="Tooltip content">
  {(radixProps) => <button {...radixProps}>Tooltip trigger</button>}
</MyTooltip>;

Why?

• I've found components that preform props injection via clone element to be brittle and not scale well over time. Something that is normally thought of as closed, a React Element, is being opened and changed by Slot.
• Explicit clarity - with children as a function it's clear that MyTrigger needs to support radixProps and it's easy for the dev to inspect/wrap/control the radixProps as needed.
• TypeScript - with as={Slot} the props that children are required to accept is not typed and it's not clear what they are. Using children as a function, typing the radixProps is fairly straight forward.

The Slot concept, which I think was popularized in Vue, is used differently in Vue than it is here in Radix. When passing props between children is necessary (which is what props injection via clone element, and children as a function do), it's done with Scoped Slots which makes the props available in the scope where the dev is working to be explicitly passed to the other child. This avoids the confusing pitfalls that props injection via clone element has, and is essentially how children as a function works (with some added syntactic sugar).

While my suggestion would be to replace Slot with children as a function, I think they can co-exist in Radix if necessary.

Also, as a side note, I was reading through some of the code and I don't think this Slottable is doing anything. The way Slot is written it only checks top-level children, and AFAICT AlertDialogContentProvider will never render as Slot.

[jjenzz]

Hi @rafgraph,

It's entirely up to you whether you use Slot or not. If you would prefer not to, this is the way we propose:

const Button = React.forwardRef((props) => <button {...props}>Tooltip trigger</button>);

<Tooltip content="Tooltip without slot">
  <TooltipTrigger as={Button} />
</Tooltip>

We're purposefully avoiding render props with our API for a few reasons:

• We like to keep a consistent approach (not multiple ways to do the same thing) to keep API surface small, less to learn etc. and we chose Slot because;
• We try to provide an API that requires near-zero wiring. Many of our users are relatively junior and they need something that Just Works:tm:. Unfortunately, we believe that render props go against this goal.
• It's impossible to React.memo a component that uses render props and while folk rarely should need React.memo, we are performance conscious and would not feel comfortable advocating (or teaching) a coding style that prevents people from making perf improvements if we can avoid it.

That's not to say we would never use/consider them though, they're just more of a last resort for us.

Explicit clarity - with children as a function it's clear that MyTrigger needs to support radixProps and it's easy for the dev to inspect/wrap/control the radixProps as needed.

If you do decide to use Slot, the component/element you slot in should accept DOM props and ref. They're the only things we pass. You shouldn't need control of these so I would be interested in hearing use-cases where you felt like you needed more control with Radix Primitives specifically?

We're aware that the ref isn't always obvious but we've heard rumour that the need for React.forwardRef might go away some day so it's an easy thing to document for now. @benoitgrelard and I have spoken about updating the docs to communicate this all more clearly.

TypeScript - with as={Slot} the props that children are required to accept is not typed and it's not clear what they are. Using children as a function, typing the radixProps is fairly straight forward.

We can perhaps look into improving types for Slot and the as prop so that the example above would error if your component didn't accept the necessary props. I'll create a Github issue to explore this so thanks for raising that 👍

Also, as a side note, I was reading through some of the code and I don't think this Slottable is doing anything

It is doing something 🙂 but this is all internal stuff that our consumers shouldn't need to be impacted by.

[rafgraph]

I don’t follow. If TooltipPrimitive.Trigger passes in a ref as part of radixProps, it will have access to the DOM element. The render prop function is just a function, not a component, so there’s no need to wrap it in forwardRef. When it’s called, it’s called as a function, not used inside of JSX.

Also just realized there’s a typo/error in @jjenzz memo example, just {children} should be passed to TooltipPrimitive.Trigger not {children(radixProps)}, don’t call the function (the TooltipPrimitive.Trigger component would do that internally). This might be throwing you off.

const MyTooltip = React.memo(({ children, content }) => (
  <TooltipPrimitive.Root>
    <TooltipPrimitive.Trigger>
       {children}
    </TooltipPrimitive.Trigger>
    <TooltipPrimitive.Content>
      {content}
      <TooltipPrimitive.Arrow />
    </TooltipPrimitive.Content>
  </TooltipPrimitive.Root>
));

[benoitgrelard]

I don’t follow. If TooltipPrimitive.Trigger passes in a ref as part of radixProps, it will have access to the DOM element. The render prop function is just a function, not a component, so there’s no need to wrap it in forwardRef. When it’s called, it’s called as a function, not used inside of JSX.

If you're rendering just a straight button element yes, but that's pretty unlikely with that pattern as I think the whole point is you're trying to use a tooltip with a button that's part of your design systems or component library already.

[rafgraph]

Ah yes, but that’s true when passing a component using Slot as well, you’ll always need to wrap it in forwardRef :)

[benoitgrelard]

Yep, which is something that already trips up some of our users.

[jjenzz]

I respect that the children as a function pattern isn't good fit for Radix :) I get lots of ideas/suggestions in my projects that I decline because I don't think they're a good fit.

❤️

I don't have specific examples of Slot causing brittleness as I haven't used it enough. I raised this as an idea because to me Slot doesn't pass the smell test, but that's just my opinion :)

Haha, I hear you. It will continue to pass our smell test though until we have proof that it is causing significant issues that weigh heavier than the DX advantages it currently provides 🙂

The need to also useCallback when optimizing with React.memo is one of the tradeoffs of the render prop pattern

Sure, that's why I was saying you cannot memoize a component with a render prop. A "render prop" to a consumer isn't {React.useCallback(() => {}, [])}, it's {() => {}} so if we introduce them, they could wind up with unintentional side-effects without realising.

If our users have to start "patching things" to avoid unintentional side-effects then we've already gone too far in the wrong direction IMO.

Regarding encapsulation, to me the cloneElement api is an escape hatch, it is sometimes needed, but generally should be avoided.

I agree it should be a last resort but Slot isn't required and it is rarely used. It's simply there to improve DX slightly if you choose to use it. You can achieve a render-prop-like approach without it:

<Tooltip content="Tooltip content">
  <TooltipTrigger as={forwardRef((radixProps) => <button {...radixProps}>Tooltip trigger</button>)} />
</Tooltip>

// vs...
<Tooltip content="Tooltip content">
  {(radixProps) => <button {...radixProps}>Tooltip trigger</button>}
</Tooltip>

Although we really wouldn't recommend creating a component inline like that, hence my original example abstracted it. The abstraction really isn't a big enough issue to warrant render props IMO. People have abstracted components purely for a className (styled-components) for years and I personally find the abstractions avoid render callback hell 🙂

There isn't much use in us continuing to discuss how render props work, we've used them for years hence deciding against them. We just don't feel that they're needed as part of Radix (yet) given that everything can be achieved fine without them and we don't believe they add any significant benefit 🙁

[rafgraph]

@jjenzz just want to make sure I understand the suggested solution if I want to avoid using Slot, is this correct?

// create MyTooltip with closed api
import * as TooltipPrimitive from '@radix-ui/react-tooltip';

export const MyTooltip = ({ children, content }) => (
  <TooltipPrimitive.Root>
    {children}
    <TooltipPrimitive.Content>
      {content}
      <TooltipPrimitive.Arrow />
    </TooltipPrimitive.Content>
  </TooltipPrimitive.Root>
);

import * as TooltipPrimitive from '@radix-ui/react-tooltip';
import { MyTooltip } from '...';

const MyButton = React.forwardRef((props) => <button {...props} />);

const App = () => (
  <MyTooltip content="Tooltip content">
    <TooltipPrimitive.Trigger as={MyButton}>Tooltip name</TooltipPrimitive.Trigger>
  </MyTooltip>
);

[benoitgrelard]

If what you want is close-over some of the implementation then yes that's one way to do it 👍
You'll probably want to forward some of the props correctly to Root and Content too then if you close-over them like that.

[jjenzz]

Yep, although if it were me I'd expose the trigger from MyTooltip so you don't have imports from two separate packages everywhere. It will be its own encapsulated package then:

// create MyTooltip with closed api
import * as TooltipPrimitive from '@radix-ui/react-tooltip';

export const MyTooltip = ({ children, content }) => (
  <TooltipPrimitive.Root>
    {children}
    <TooltipPrimitive.Content>
      {content}
      <TooltipPrimitive.Arrow />
    </TooltipPrimitive.Content>
  </TooltipPrimitive.Root>
);

export const MyTooltipTrigger = TooltipPrimitive.Trigger;

import { MyTooltip, MyTooltipTrigger } from '...';

const MyButton = React.forwardRef((props) => <button {...props} />);

const App = () => (
  <MyTooltip content="Tooltip content">
    <MyTooltipTrigger as={MyButton}>Tooltip name</MyTooltipTrigger>
  </MyTooltip>
);

[rafgraph]

Thanks @jjenzz, I like that better!

[benoitgrelard]

Yep, that would be my preferred way too 👍