RFC: update slots API to allow arrays #19266
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 32a068f:
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 5369d330c7b0983efba35c6e945340506007c6c8 (build) |
📊 Bundle size reportUnchanged fixtures
|
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 851 | 879 | 5000 | |
| BaseButton | mount | 852 | 868 | 5000 | |
| Breadcrumb | mount | 2610 | 2604 | 1000 | |
| ButtonNext | mount | 428 | 426 | 5000 | |
| Checkbox | mount | 1510 | 1469 | 5000 | |
| CheckboxBase | mount | 1273 | 1267 | 5000 | |
| ChoiceGroup | mount | 4637 | 4647 | 5000 | |
| ComboBox | mount | 955 | 965 | 1000 | |
| CommandBar | mount | 9982 | 10021 | 1000 | |
| ContextualMenu | mount | 6043 | 6141 | 1000 | |
| DefaultButton | mount | 1118 | 1077 | 5000 | |
| DetailsRow | mount | 3624 | 3632 | 5000 | |
| DetailsRowFast | mount | 3660 | 3669 | 5000 | |
| DetailsRowNoStyles | mount | 3421 | 3480 | 5000 | |
| Dialog | mount | 2118 | 2159 | 1000 | |
| DocumentCardTitle | mount | 132 | 144 | 1000 | |
| Dropdown | mount | 3180 | 3152 | 5000 | |
| FluentProviderNext | mount | 7433 | 7396 | 5000 | |
| FocusTrapZone | mount | 1756 | 1745 | 5000 | |
| FocusZone | mount | 1798 | 1790 | 5000 | |
| IconButton | mount | 1704 | 1707 | 5000 | |
| Label | mount | 329 | 330 | 5000 | |
| Layer | mount | 1758 | 1752 | 5000 | |
| Link | mount | 446 | 450 | 5000 | |
| MakeStyles | mount | 1812 | 1783 | 50000 | |
| MenuButton | mount | 1420 | 1419 | 5000 | |
| MessageBar | mount | 1998 | 2007 | 5000 | |
| Nav | mount | 3203 | 3213 | 1000 | |
| OverflowSet | mount | 1023 | 1006 | 5000 | |
| Panel | mount | 2103 | 2038 | 1000 | |
| Persona | mount | 811 | 812 | 1000 | |
| Pivot | mount | 1408 | 1372 | 1000 | |
| PrimaryButton | mount | 1249 | 1276 | 5000 | |
| Rating | mount | 7491 | 7442 | 5000 | |
| SearchBox | mount | 1329 | 1275 | 5000 | |
| Shimmer | mount | 2455 | 2497 | 5000 | |
| Slider | mount | 1938 | 1875 | 5000 | |
| SpinButton | mount | 4819 | 4845 | 5000 | |
| Spinner | mount | 411 | 417 | 5000 | |
| SplitButton | mount | 3053 | 3095 | 5000 | |
| Stack | mount | 496 | 484 | 5000 | |
| StackWithIntrinsicChildren | mount | 1559 | 1537 | 5000 | |
| StackWithTextChildren | mount | 4479 | 4494 | 5000 | |
| SwatchColorPicker | mount | 10173 | 9980 | 5000 | |
| Tabs | mount | 1356 | 1384 | 1000 | |
| TagPicker | mount | 2531 | 2538 | 5000 | |
| TeachingBubble | mount | 11739 | 11653 | 5000 | |
| Text | mount | 402 | 398 | 5000 | |
| TextField | mount | 1346 | 1317 | 5000 | |
| ThemeProvider | mount | 1159 | 1183 | 5000 | |
| ThemeProvider | virtual-rerender | 596 | 601 | 5000 | |
| Toggle | mount | 784 | 801 | 5000 | |
| buttonNative | mount | 115 | 120 | 5000 |
Perf Analysis (@fluentui/react-northstar)
Perf tests with no regressions
| Scenario | Current PR Ticks | Baseline Ticks | Ratio |
|---|---|---|---|
| FormMinimalPerf.default | 407 | 365 | 1.12:1 |
| TreeWith60ListItems.default | 175 | 159 | 1.1:1 |
| ButtonMinimalPerf.default | 168 | 156 | 1.08:1 |
| AnimationMinimalPerf.default | 422 | 394 | 1.07:1 |
| CardMinimalPerf.default | 566 | 527 | 1.07:1 |
| CarouselMinimalPerf.default | 458 | 433 | 1.06:1 |
| ButtonSlotsPerf.default | 558 | 531 | 1.05:1 |
| ChatWithPopoverPerf.default | 367 | 349 | 1.05:1 |
| LabelMinimalPerf.default | 386 | 367 | 1.05:1 |
| ListCommonPerf.default | 620 | 592 | 1.05:1 |
| SegmentMinimalPerf.default | 354 | 337 | 1.05:1 |
| StatusMinimalPerf.default | 668 | 636 | 1.05:1 |
| AccordionMinimalPerf.default | 155 | 149 | 1.04:1 |
| DropdownManyItemsPerf.default | 658 | 630 | 1.04:1 |
| PopupMinimalPerf.default | 594 | 571 | 1.04:1 |
| PortalMinimalPerf.default | 179 | 172 | 1.04:1 |
| TextMinimalPerf.default | 350 | 337 | 1.04:1 |
| AvatarMinimalPerf.default | 189 | 183 | 1.03:1 |
| DropdownMinimalPerf.default | 3089 | 3012 | 1.03:1 |
| ProviderMergeThemesPerf.default | 1692 | 1639 | 1.03:1 |
| DatepickerMinimalPerf.default | 5312 | 5232 | 1.02:1 |
| FlexMinimalPerf.default | 283 | 277 | 1.02:1 |
| ListMinimalPerf.default | 501 | 489 | 1.02:1 |
| ReactionMinimalPerf.default | 372 | 364 | 1.02:1 |
| TableManyItemsPerf.default | 1844 | 1812 | 1.02:1 |
| TextAreaMinimalPerf.default | 478 | 468 | 1.02:1 |
| AlertMinimalPerf.default | 271 | 267 | 1.01:1 |
| DialogMinimalPerf.default | 733 | 723 | 1.01:1 |
| EmbedMinimalPerf.default | 4058 | 4021 | 1.01:1 |
| HeaderSlotsPerf.default | 743 | 733 | 1.01:1 |
| ItemLayoutMinimalPerf.default | 1196 | 1186 | 1.01:1 |
| RefMinimalPerf.default | 237 | 235 | 1.01:1 |
| SkeletonMinimalPerf.default | 339 | 334 | 1.01:1 |
| BoxMinimalPerf.default | 333 | 333 | 1:1 |
| ButtonOverridesMissPerf.default | 1665 | 1662 | 1:1 |
| ChatMinimalPerf.default | 643 | 644 | 1:1 |
| GridMinimalPerf.default | 311 | 310 | 1:1 |
| ImageMinimalPerf.default | 354 | 354 | 1:1 |
| InputMinimalPerf.default | 1229 | 1228 | 1:1 |
| LayoutMinimalPerf.default | 356 | 356 | 1:1 |
| ListWith60ListItems.default | 625 | 627 | 1:1 |
| LoaderMinimalPerf.default | 683 | 683 | 1:1 |
| SliderMinimalPerf.default | 1565 | 1567 | 1:1 |
| SplitButtonMinimalPerf.default | 3716 | 3708 | 1:1 |
| IconMinimalPerf.default | 608 | 608 | 1:1 |
| CustomToolbarPrototype.default | 3809 | 3800 | 1:1 |
| TooltipMinimalPerf.default | 989 | 989 | 1:1 |
| TreeMinimalPerf.default | 774 | 777 | 1:1 |
| CheckboxMinimalPerf.default | 2669 | 2687 | 0.99:1 |
| ListNestedPerf.default | 537 | 545 | 0.99:1 |
| ProviderMinimalPerf.default | 992 | 1001 | 0.99:1 |
| RadioGroupMinimalPerf.default | 442 | 445 | 0.99:1 |
| TableMinimalPerf.default | 401 | 406 | 0.99:1 |
| ToolbarMinimalPerf.default | 907 | 920 | 0.99:1 |
| AttachmentMinimalPerf.default | 148 | 151 | 0.98:1 |
| MenuMinimalPerf.default | 814 | 827 | 0.98:1 |
| MenuButtonMinimalPerf.default | 1591 | 1623 | 0.98:1 |
| AttachmentSlotsPerf.default | 1023 | 1052 | 0.97:1 |
| DividerMinimalPerf.default | 336 | 346 | 0.97:1 |
| HeaderMinimalPerf.default | 350 | 359 | 0.97:1 |
| ChatDuplicateMessagesPerf.default | 289 | 300 | 0.96:1 |
| RosterPerf.default | 1152 | 1201 | 0.96:1 |
| VideoMinimalPerf.default | 616 | 642 | 0.96:1 |
| ...etc, | ||
| components: { | ||
| root: 'div', | ||
| options: 'div', |
There was a problem hiding this comment.
One drawback here is that you can't specify a different component for each entry in the array. In this case, every option is a <div>, and there's no way to make certain ones be a <GroupHeader> component or something.
This comment is mostly an FYI because I think I have a decent solution that modifies how this components prop works. I'm going to write an RFC with this proposal, but a quick summary:
// Currently requires a components={...} object separate from the slot value
<MyButton components={{ label: MyLabel }} label="foo" />
<MyButton components={{ label: 'span' }} label={{ className: 'baz', children: 'bar' }} />
// Proposal: slots could accept a tuple of [ type, shorthandValue ]
<MyButton label={[ MyLabel, 'foo' ]} />
<MyButton label={[ 'span', { className: 'baz', children: 'bar' } ]} />That would naturally allow a different component to be specified per slot in this RFC:
<Combobox options={[
[GroupHeader, 'Fruits'],
'apple',
'banana',
[GroupHeader, 'Vegetables'],
'carrot',
'daikon',
]} />|
|
||
| ```js | ||
| export function resolveShorthandArray<Props extends Record<string, any>>( | ||
| value: ShorthandProps<Props>[], |
There was a problem hiding this comment.
You'd also want to accept undefined | null here:
| value: ShorthandProps<Props>[], | |
| value: ShorthandProps<Props>[] | undefined | null, |
| value: ShorthandProps<Props>[], | ||
| defaultProps?: Props, | ||
| ): ObjectShorthandProps<Props>[] { | ||
| if (value === null || !Array.isArray(value)) { |
There was a problem hiding this comment.
If you take my suggestion above, you'd also need to check for undefined (or just falsy). Also, (nit) remove the isArray check; that should be handled by typescript.
| if (value === null || !Array.isArray(value)) { | |
| if (!value) { |
| } | ||
| ``` | ||
|
|
||
| One downside to this solution is it that in order for an author to define a set of props that appear on every option, they'll need to duplicate that prop object for each option in `props.options`. We could potentially consider adding another prop like `defaultOption: React.HTMLAttributes<HTMLElement>` to make that easier. |
There was a problem hiding this comment.
I think it's ok to start by requiring authors to manually apply a prop to each entry for now (e.g. using map). If it becomes a problem, we could add another way of doing it in the future.
| Than this: | ||
|
|
||
| ```js | ||
| <Combobox> |
There was a problem hiding this comment.
Can we update this example to showcase usage? IMO this comparison will be more accurate:
const options = ["apple", "banana", "cucumber"];
function App() {
return (
<>
<Combobox options={options} />
{/* VS. */}
<Combobox>
{options.map(item => (
<Option key={item}>{item}</Option>
))}
</Combobox>
</>
);
}It's very similar to what React docs are showing: https://reactjs.org/docs/lists-and-keys.html#basic-list-component
There was a problem hiding this comment.
The map does not need to be rendered directly as children, this will now resemble the shorthand proposal quite closely, and allows easy memoization
const options = ["apple", "banana", "cucumber"];
function App() {
const comboboxItems = options.map(item => (<Option key={item.id}>{item.name}</Option>))
return (
<>
<Combobox>
{comboboxItems}
</Combobox>
</>
);
}
There was a problem hiding this comment.
I prefer this approach to having two props of items and onRenderItems which is what we did in v8. It's effectively the same thing, but the mapping is done by the user instead of the component.
| return ( | ||
| <slots.root {...slotProps.root}> | ||
| {slots.options.map((Option, i) => { | ||
| return <Option key={`option-${i}`} {...slotProps.options[i]} />; |
There was a problem hiding this comment.
Using Index as a key is an anti-pattern in React:
We don’t recommend using indexes for keys if the order of items may change. This can negatively impact performance and may cause issues with component state. Check out Robin Pokorny’s article for an in-depth explanation on the negative impacts of using an index as a key. If you choose not to assign an explicit key to list items then React will default to using indexes as keys.
https://reactjs.org/docs/lists-and-keys.html#keys
https://robinpokorny.medium.com/index-as-a-key-is-an-anti-pattern-e0349aece318
|
|
||
| ## Summary | ||
|
|
||
| This RFC proposes modifying the Slots API to accept arrays of slot props to support components like Combobox or List, where the most ergonomic authoring experience is passing in an array of data rather than unique JSX children. |
There was a problem hiding this comment.
This is how we started in Semantic UI React and we decided to use the same API in Northstar, we call it shorthand collections. When we started there were no other way to pass props from parent to children components:
- React Context was added in React 16.3
- React Hooks in 16.8
- while Semantic UI React started with React 15 🙄
From our POV this API worked well as it's really simple:
// 🦁 wow, so easy!
<Dropdown items={["Robert Tolbert", "Wanda Howard", "Tim Deboer"]} />However, usage in complex applications that are our main audience is different: data comes from different sources, lists are huge and there strict performance requirements. Below is a summary of feedback that we received from Northstar customers.
Keys are hard
First of all, customers should use keys (which is not usually obvious in examples):
<>
// 🟥 if we will use these strings a keys, we will get a React warning about items with duplicate keys
<Dropdown items={["Robert", "Robert"]} />
// ✅ will not produce warnings
<Dropdown
items={[
{ key: "robert-k", content: "Robert" },
{ key: "robert-t", content: "Robert" }
]}
/>
</>There is no easy option how generate keys for customers in automated way, they should pass it. Check a comment from @levithomason in Semantic-Org/Semantic-UI-React#2418 (comment), it's a good summary from library POV.
Performance issues (memoization)
"My items are really expensive to render, how I memoize them?"
Items can really expensive to re-render especially in components like Dropdown, Menu, List & etc. A simple Avatar from Teams is below, it's even without status 😱
One of the frequent asks that we had was about memoization, we have an entry in our FAQ. Developers want to implement their features and expect that library will work intuitively for them. But it does not seem to be true from an example below:
const MemoizedItem = React.memo(Menu.Item, (prevProps, nextProps) => {
return prevProps.active === nextProps.active;
});
const renderBlocker = (Component, props) => <MemoizedItem {...props} />;
function App() {
return (
<>
{/* 👎 Even with `renderItem()` it will not be better */}
<Menu
items={[
{ content: "One", key: "one", children: renderBlocker },
{ content: "Two", key: "two", children: renderBlocker }
]}
/>
{/* 👍 This looks like in React docs */}
<Menu>
<MemoizedItem>One</MemoizedItem>
<MemoizedItem>Two</MemoizedItem>
</Menu>
</>
);
}Performance issues (redundant rendering)
Another feedback that we received is related to a fact that customers should compute their items in advance for menus (or dropdowns, etc.) even before they are visible.
It could be cheap to get data for the items itself, but each item can contain a lot of additional info required to render: user's state, description, etc. For example:
<>
<Dropdown
items={[
{
// 👇 "key" & "content" are required for rendering
key: "robert-t",
content: "Robert Tolbert",
// 👇 "image" & "description" could be fetched later, but we don't allow this with "items" 😕
image: "/avatar/RobertTolbert.jpg",
description: "Software Engineer"
}
]}
/>
<Dropdown>
{/* 👇 only minimal data to render "Dropdown" */}
{[{ key: "robert-t", content: "Robert Tolbert" }].map(item => (
// 👍 Data can be fetched lazily inside custom component
<DropdownItemThatFetchesDetails {...item} />
))}
</Dropdown>
</>A short demo of this behavior is available on this CodeSandbox:
- for v0 we should call
getUserStatus()before a menu is opened - for v9 we call
getUserStatus()only when a menu is opened
Performance issues (root of update is moved to the parent)
Another issue that we met is related to a fact that a parent component is a root of rendering. It's highly coupled to a previous section. See an example below:
<>
<Dropdown
// 👇 "online" is a property of an item, if we will change it, Dropdown should be re-rendered *anyway*
// 💥 Other items will be also re-rendered by default, to avoid this we will need to use memoization
items={[{ key: "robert-t", content: "Robert Tolbert", status: "online" }]}
/>
<Dropdown>
{[{ key: "robert-t", content: "Robert Tolbert" }].map(item => (
// 😅 As "status" is computed *inside* an item, only it will be updated
<DropdownItemWithStatus {...item} />
))}
</Dropdown>
</>Demo of this behavior on CodeSandbox, check output in console:
// with "items"
render:DropdownA
render:DropdownItemA Jane
render:DropdownItemA John
// with JSX
render:DropdownItemBJane
Bundle size penalty
If I don't use DropdownItem from Fluent, it will be bundled anyway with Dropdown as it calls it internally:
<>
<Dropdown
// 👇 somewhere internally we need to render "DropdownItem", it's a direct dependency
// and will be bundled anyway
items={[{ key: "robert-t", content: "Robert Tolbert", status: "online" }]}
/>
<Dropdown>
{/* 👇 Can be absolutely custom and used custom styles without penalties */}
<CustomItem />
</Dropdown>
</>Virtualization
Components like Dropdown (Combobox) usually have filtering (client search) on client-side this means that input items are different from output items:
This means that virtualization component should be put inside Dropdown to know about its state, so we will need to add more props only for it:
<>
<Dropdown
items={items}
renderItem={item => <MemoizedItem {...item} />}
renderItems={(
jsxItems /* items from "renderItem()" */,
rawItems /* complex lists can require item objects */
) => (
<List height={150} itemCount={items.length} itemSize={35} width={300}>
{items}
</List>
)}
>
{/* 💡 this should be inside */}
</Dropdown>
{/* VS. */}
<Dropdown>
<List height={150} itemCount={items.length} itemSize={35} width={300}>
{MemoizedItem}
</List>
</Dropdown>
</>It's one of reasons why we refactored Tree component in Northstar to hooks, it was to complex and not enough customisable.
BTW, a I don't think that we should include virtualization into components by default:
- our customers has own solutions
- it will bundle size penalty for components
Libraries that were designed after React hooks appeared also use JSX children in the most cases:
- https://evergreen.segment.com/components/select
- https://chakra-ui.com/docs/form/select
- https://reach.tech/combobox
- https://material-ui.com/components/selects/
- https://react-spectrum.adobe.com/react-spectrum/ComboBox.html (supports both)
This happened mainly because of React Context & hooks, these APIs simplified sharing data & handlers between parent and children components. We also use use-context-selector to avoid redundant updates in child components, there is a good showcase for it in Northstar docs (only items with updated state are re-rendered).
For components like Combobox I suggest to keep going with JSX items. We will meet edge cases like Tree component where items are nested and it will be impossible to use JSX items, but I am considering this as an exception rather than a rule.
There was a problem hiding this comment.
Thanks for the detailed comment! The issues with performance and memoization are great to bring up. I'm curious about the best way to approach some of the issues I ran into when implementing Combobox with children, especially now that the Descendants API has been removed.
Essentially, the biggest issue is that the parent control (or a parent context) needs to be aware of the number of child options present, as well as their key/id values so that the active item can be controlled. The parent is the one that receives the keyboard events, and the one that needs to set the current item's id in an attribute, so it needs the ability to get the previous/next/first/last/etc child's id in response to user interaction.
Theoretically this could work with React.children, as long as it's a flat list. The issue there is something like this:
<Combobox>
<Group name="fruits">
<Option>apple</Option>
<Option>banana</Option>
</Group>
<Group name="vegetables">
<Option>onion</Option>
<Option>zucchini</Option>
</Group>
</Combobox>Potentially we could do something like create a context that gives each option the ability to register/deregister itself by id, though it would be difficult to ensure we have the ids in the correct order. There's maybe also something we can do with React.Children in both Combobox and Group, so Combobox's context ends up with the correct set of current options. No matter what we do, the parent is going to need to update any time the descendants array updates, just based on how activedescendant works. There's likely a clever solution to this somewhere that I haven't been able to think of yet 😊.
For memoization, since we're dealing with slot arrays, I also wonder if authoring like this might work:
const MemoOption = React.memo(Option);
<Combobox options=[
<MemoOption props={...etc} />
<MemoOption props={...etc} />
<MemoOption props={...etc} />
] />I need to test it more, but it seems like that might still allow delayed updates and allowing the author to update one child without affecting the others. I'm definitely not confident on that approach yet, I'd definitely need to test it more (or you might know offhand if there's a reason it wouldn't fly). It definitely makes managing Combobox's state easier though 😅.
There was a problem hiding this comment.
Virtualization is also a really interesting case, since I think even with options as children, the virtualization provider would likely need to live inside Combobox as a slot, since Combobox itself also needs to know what the total number of options is vs. the rendered number.
There was a problem hiding this comment.
Chatted @smhigley and @behowell about this RFC. After pondering a bit, I think having a slot that takes an array of literals, props objects, or Components is what I would expect as consistent with singular slots in other components. A common scenario will be to pass in the array of props and be able to have a Component that is given each of the those props as a template. What if there was an itemAs property that behaved like as (literal tag name, component, or render function) that would be used for each item? This extends the as metaphor from single slots to a slot with an array.
There was a problem hiding this comment.
the virtualization provider would likely need to live inside Combobox as a slot, since Combobox itself also needs to know what the total number of options is vs. the rendered number.
With the v9 hooks/render func composition architecture, I would have expected users to simply create their own component variants and wrap children with the virtualization provider
| The authoring experience would look like this: | ||
|
|
||
| ```js | ||
| <Combobox option={{ 'some-default-prop-on-all-options': 'test' }} data={['apple', 'banana', 'cauliflower']}> |
There was a problem hiding this comment.
Having been a web-dev using OUIF, this kind of shorthand API was extremely unfriendly to deal with especially when you need to support features like:
- Async loading
- Special actions rerendering a specific item
- Slightly altering specific items but not all
OUIF was very well known to have a lot of onRenderX callbacks or specific onXLifecycle methods to support this kind of shorthand API, which not only resulted in more code, but actually required mappings from our own application state to specific component lifecycle methods
| In contrast, Combobox and Listbox patterns do commonly take an array of data and use it to render mostly uniform items. The same is likely going to be true of components like List (if we have one) and DetailsList/Grid. For those controls, good authoring ergonomics lean more towards this: | ||
|
|
||
| ```js | ||
| <Combobox options={['apple', 'banana', 'cucumber', ...etc]} /> |
There was a problem hiding this comment.
Many combobox/dropdown examples that I have seen in Teams, were not this library friendly. Most items could not just be mapped to simple strings, but also required icons, specific layout changes, styles overrides etc..
There was a problem hiding this comment.
@ling1726 thanks for the feedback! Does it help at all that options is a slot array and not a string array? It means items could be authored as strings, slot objects, or JSX elements without needing an onRenderX function.
There was a problem hiding this comment.
For the RFC I would propose that it also provides some code samples of these more complex options content
behowell
modified the milestones:
August Project Cycle Q3 2021,
September Project Cycle Q3 2021
|
Sometime ago we had an offline conversation with @smhigley & @behowell, I promised to prototype a solution that will allow to avoid using arrays aka shorthand collections.
|
|
Adding this as a comment so it doesn't get lost: One thing we talked about in the Combobox/Dropdown sync is creating a POC for a @behowell can correct me if I missed anything important 😄 |
Please link a POC PR when there is one, thanks ! |
|
Combobox ended up using children for items so we don't have a pressing need to solve this problem now. We can resurrect this if we encounter a need later. |
This RFC looks at possibilities for how to handle components like Combobox, List, or Table/Grid that commonly take arrays of data rather than individual slots or manually authored children.