Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Slots #223

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

RFC: Slots #223

wants to merge 4 commits into from

Conversation

nihgwu
Copy link

@nihgwu nihgwu commented Aug 5, 2022

In this RFC, we propose a way to support Slots pattern in React

View formatted RFC

Here is a demo how it would work I implemented with react-call-return in 10 lines

@tom-sherman
Copy link

tom-sherman commented Aug 6, 2022

I'm very interested in this proposal, being able to build your own node tree and have it managed by react is very powerful.

The string-name based API makes this hard to impossible to statically type. This is something the context API gets right but I feel is missing in this proposal.

Do you have any thoughts on this? Maybe a createContext like API is better?

const {Host, Slot} = createSlot()

@nihgwu
Copy link
Author

nihgwu commented Aug 6, 2022

@tom-sherman for typings, you will have to do const Slot = createSlot<Props>(), in my own package it's automatically inferred, works the best with TS, but here I want to make it more generic and lightweight, so you will have to provide types manually, and it's just a one-time work, but you remind me an idea to improve the api, that we don't need string based slot name at all, tag type already does the job, will update soon.

Regarding Host Slot pair, I don't think it will help improving typings as we will have multiple slots while each one accept different props, like LabelSlot<LabelProps> and InputSlot<InputProps>

@nihgwu nihgwu force-pushed the neo/slots branch 3 times, most recently from f2e09b7 to cb90170 Compare August 6, 2022 23:38
@tom-sherman
Copy link

tom-sherman commented Aug 8, 2022

The issue with the string-based API was that there is no easy way to say that a string ID relates to a particular slot type. My proposed API was just an example to demonstrate the concept of having the callsite and the definition site be statically linked which your new API also does but in a different way 👍

It would be good if you could add some examples that address the following scenarios:

  • Non-slot children being rendered alongside or inside of slot children
  • Rendering a slot inside a slot

@nihgwu
Copy link
Author

nihgwu commented Aug 8, 2022

@tom-sherman Added

@nihgwu
Copy link
Author

nihgwu commented Aug 25, 2022

here is the demo to build a tree from Slots, with create-slots vs with react-call-return
Checkout the console.log you will find how clean it will be if it's built in React core, as in user space we have no way to know the index when inserting an item, so I have to rescan the children of a host, then it results in lots of force update

@stefee
Copy link

stefee commented Aug 25, 2022

This is an interesting proposal. I'm a relatively uninformed reader of this, so perhaps an ignorant question:

I'm trying to understand what is enabled by this that is not possible using props. For example:

function MyComponent({ firstEl, lastEl }) {
  return <div>{firstEl}<p>Hello</p>{lastEl}</div>;
}

The 'slots' here are passed as React elements into the component as props.

@tom-sherman
Copy link

tom-sherman commented Aug 25, 2022

@stefee This is a completely legitimate confusion, I think the RFC needs to improve a bit on the example side to showcase what problem this solves at a glance.

My mental model for this problem space is based around the <select>/<option> component API, this is excellently described in @reach-ui/descendants: https://github.com/reach/reach-ui/tree/dev/packages/descendants#the-problem

I think it would be worth bringing this example into this RFC pretty much verbatim.

Another important note is that this is an API targeted primarily at library developers. I wouldn't expect to see it used too much in apps.

Hopefully this explains it better!

@nihgwu
Copy link
Author

nihgwu commented Aug 25, 2022

@stefee that's a good question, TLDR: yes you can do it and that's the cheapest approach for app devs, but for ui library authors it's not a good API IMO, and the proposal is much powerful, like A11y and virtualisation support.

Think about how would you support A11y in your approach? Consume context in each slot? Which is the approach for most libraries like radix-ui, but you can see how simple it is with this proposal, the id is already in the JSX tree, why do we need a double render to get it? IoC means a lot.

In terms of Virtualisation, you can find more context in this thread, but you can take it as a tree info collector without rendering the tree to real DOM, so it will enable more possibilities.

Also this proposal is much closer to the Slots in Web Components and we don't need to use a static prop(slot) to achieve that.

@nihgwu
Copy link
Author

nihgwu commented Aug 25, 2022

@tom-sherman Yes I agree the examples need to be improved, but I'm afraid it will prolong the content a lot if I include the content of https://github.com/reach/reach-ui/tree/dev/packages/descendants#the-problem while I've provide the links there, but probably I should highlight those links to make them more accessible

@tom-sherman
Copy link

tom-sherman commented Aug 25, 2022

Understood, maybe just highlighting the link more in a more prominent position is the way to go.

To be clear I wasn't suggesting including the different "options" from that doc, just the problem statement section. I think <Menu>/<MenuItem> is the easiest to understand use case.

@jlarmstrongiv
Copy link

Slots are definitely a feature I miss in React, having worked with Vue and Web Components.

First-party support for the Slots API would be great! This RFC would avoid the force-updates and enable slots to be used with React Server Components.

@giuseppelt
Copy link

Just as proof-of-concept, I wrote a very simple Slot component.
https://www.breakp.dev/blog/simple-slot-system-for-react/

Maybe not the best solution, but just something to add into the mix.

@desko27
Copy link

desko27 commented Sep 28, 2023

This is an excellent idea. I've been using React for years and I feel like slots could've helped me in a variety of scenarios where it felt too hacky to achieve true composability. It can be done, but it's sometimes not easily, and definitely not clean.

I mean, articles like the one from @kentcdodds show the unquestionable benefits of composable components, but trying to build one can make you feel kind of abandoned/unsupported by the framework itself, as if you were going against it.

Today I found create-slots and this RFC. I just started using it and I built a clean composable card component in no time. Feels very native and looks so maintainable. Also TS support is excellent.

A nice API that solves a real problem. This is the way. TY @nihgwu, I'd love to see this built in React.

@Flammae
Copy link

Flammae commented Nov 22, 2023

As someone who independently developed my solution for integrating slots into React even before discovering this RFC, I would like to discuss my implementation here to bring a fresh perspective and talk about some other challenges with this and my own design.

My implementation focuses on a simplistic and type-safe API, closely resembling implementations from other frameworks, especially Vue.js.

Implementation

type ListItemProps = {
  children: SlotChildren<
    | Slot<"title"> // Shorthand of Slot<"title", {}>
    | Slot<"thumbnail"> // Shorthand of Slot<"thumbnail", {}>
    | Slot<{ isExpanded: boolean }> // Shorthand of Slot<"default", {isExpanded: boolean}>
  >;
};

function ListItem({ children }: ListItemProps) {
 const { slot, hasSlot } = useSlot(children); // Slot and hasSlot object inferred from children.

 return (
   <li>
     {/* Render thumbnail if provided, otherwise nothing*/}
     <slot.thumbnail />
     <div>
       {/* Render a fallback if title is not provided*/}
       <slot.title>Expand for more</slot.title>
       {/* Render the description and pass the prop up to the parent */}
       <slot.default isExpanded={dynamicValue} />
     </div>
   </li>
 );
}

Here, I used a build plugin to change slot elements into function invocations at build time. This is because slots require access to children with closure and thus need to be created for every component. If this feature were built into React, useSlot would not need to accept children as an argument but would be able to access it with a context-like API, eliminating the need for a custom build plugin. Slots could also just be a function, but syntactic sugar is just too good to say no to.

Cool Things (So Far):

  • We type children directly. While TypeScript can't enforce a strict shape for JSX, it can still help enforce strict type checking for strings, numbers, functions (implicit default slots). It can also strictly type check elements created with React.createElement.

Drawbacks (So Far):

  • Already more opinionated than OP implementation

Usage from parent

slot-name attribute (Not type-safe)

<ListItem>
  <img slot-name="thumbnail" src="..." />
  <div slot-name="title">A title</div>
  this is a description.
</ListItem>

Everything with a slot-name attribute goes inside its corresponding slots. It's hyphenated because TypeScript doesn't enforce hyphenated attributes, which is important when you want to specify slot-name on a custom component. Content without a slot-name attribute implicitly lands in the default slot.

Templates (Optionally type-safe)

import { template } from "...";
// Or 
import { createTemplate } from "...";
const template = createTemplate<ListItemProps["children"]>();
 
<ListItem>
  <template.thumbnail>
    <img src="..." />
  </template.thumbnail>
  <template.title>A title</template.title>
  <template.description>
    {({ isExpanded }) =>
      isExpanded ? <strong>A description</strong> : "A description"
    }
  </template.description>
  <template.description>doesn't have to be a function</template.description>
</ListItem>

Templates are adopted from Vue's design. They can be made type-safe with createTemplate, and they also provide a way to access props passed up from a child, which is also common in other frameworks.

Drawbacks:

  • Templates aren't rendered; they just specify content for corresponding slots. Instead, fragments are rendered in their place to maintain key equality.

Enforcing the Node Type and Advanced Logic

In my library, there are OverrideNode elements that can be included as a direct child of slot elements to specify advanced override logic for specific slots and their fallbacks. I won't get into it here because I believe the OverrideNode API is too funky and opinionated for React. Instead, a similar API can be implemented this way, if it ever was considered to be used in this PR:

 function ListItem({ children }: ListItemProps) {
  const { slot, hasSlot } = useSlot(children); // Slot and hasSlot object inferred from children.
  
  // Called for Each node for this slot
  const SlotWithOverride = React.Children.overrideSlot(slot.thumbnail, (node) => {
    // Enforce final node type
    // Add props
    // Change type
  })
 
  return (
    <li>
      {/* Render thumbnail if provided, otherwise nothing*/}
      <SlotWithOverride />
      <div>
        {/* Render a fallback if title is not provided*/}
        <slot.title>Expand for more</slot.title>
        {/* Render the description and pass the prop up to the parent */}
        <slot.default isExpanded={dynamicValue} />
      </div>
    </li>
  );
}

This design also opens the door for type-safe composition between two slotted components. EG: Render parent-provided content for the default slot inside the child's "foo" slot and pass up or override the props specified on a child's slot. I will also not get into this here.

Final Thoughts

There are a couple of things with the slot pattern that break how rendering and existing APIs work in React:

  • My implementation, and I believe the OP's implementation (to a lesser degree), cannot meaningfully work with React.Children APIs. This is because the shape of children is unique to useSlot (mine) or createHost (OP's), and it can only be understood fully within the context of our wrappers. In my case, React.Children APIs need to prevent any kind of modification of templates and their children, potentially by skipping the iteration. This is especially true for templates with functional children because the function needs to only be called internally.
  • Because children is flattened and mapped over to collect the content for specific slots, this changes how the keys should work for entire children. Here's the link to the docs that explains this drawback in my own API. I believe this is also true for this implementation as well.

I'm very passionate about slots and the fresh new patterns it brings to our toolchain. My point was not to undermine this RFC but to bring the new ideas in and document issues that there might be to make it even better. I would love it if React supported slots natively.

If anyone wants to check out my implementation, it can be found here

@nihgwu
Copy link
Author

nihgwu commented Nov 23, 2023

@Flammae Thanks for the details comment, will take more time to understand your lib

Because children is flattened and mapped over to collect the content for specific slots

this is not the case for my impl, I avoided iteration and only rely on React's render mechanism to collect slots, so it just works as long as you don't render extra nodes around slots, so the following still works as tooltip is rendered by createPortal

const Item = () =>  <Tooltip><List.Item /></Tooltip>
const list = <List><Item /></List>

@Flammae
Copy link

Flammae commented Nov 23, 2023

What happens if the tooltip was used this way:

<List>
  {[
    <Item key={1}>First</Item>,
    <Item key={2}>Second</Item>
  ]}
  {[
    <Item key={1}>Third</Item>,
    <Item key={2}>Fourth</Item>
  ]}
  <Item key={1}>Fifth</Item>
  <Item>Without key</Item>
</List>

And what if the tooltip accepted another slot, Thumbnail, and it was inserted and reordered this way on the subsequent render:

<List>
   {[
    <Item key={2}>Fourth</Item>,
    <Thumbnail key={3} />,
    <Item key={1}>Third</Item>
  ]}
  <Item key={1}>Fifth</Item> 
  <Item>Without Key</Item>
  {[
    <Item key={2}>Second</Item>,
    <Item key={1}>First</Item>,
    <Item key={3}>New Item</Item>
  ]}
</List>

Would it be able to correctly identify which nodes to update and which nodes to re-render?

The correct solution should do this:

  • Items should be updated to be in the order specified: Fourth, Third, Fifth, Without Key, Second, First, New Item
  • The Item without key should re-render, "New Item" should mount, the other items, including "Fifth" should just be updated
  • Thumbnail should render in its own slot

Also, if our goal is to not break existing React logic, the items that were previously inside the array and on the subsequent render were outside, or nested deeper, should re-render even if the keys are the same.

To clarify, by re-render I mean unmount and remount

Edit:

I was wrong in thinking React maintains key equality when arrays themselves change positions. What's important is to maintain key equality when arrays stay in the same position and items within the array move. This implementation definitely handles Key equality right!:

Here's the test: (npm run test)
https://stackblitz.com/edit/vitejs-vite-cbevwa?file=src%2FSelect.jsx,package.json,test%2Findex.test.jsx,src%2FApp.jsx,public%2Fvite.svg&terminal=dev

@nihgwu
Copy link
Author

nihgwu commented Nov 23, 2023

@Flammae for your first question, you can try it here

for the second one, you can try it locally, the thumbnail will be rendered outside of list, and you will see a warning in console on dev env

@dante01yoon
Copy link

Do we have any further discussion so far?

@dante01yoon
Copy link

Or any plan to implement this in upcoming React release (19, 20..?)

@dante01yoon
Copy link

@will-stone do you have any reason just put thumbs down?

@will-stone
Copy link

@dante01yoon There will be lots of people subscribed to this thread, and you just pinged them all, thrice. It’s generally best not to ask for updates on these things, as updates will be posted here when they are ready. Asking for updates is like posting “+1” and doesn’t benefit the discussion. Thanks 🙂 (and no need to reply to this message, which will notify everyone again 😅).

@dante01yoon
Copy link

Agree in some aspect, but someone may want to know any further discussion happens in somewhere place since discussion suddenly hanged since end of last year. As this didn't mean to spam, IMHO we shouldn't stop those want to ask any updates. Anyways, thanks again for your kindness.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants