From a4ca057a81fdd42d40c50c5d4d3464ee38780966 Mon Sep 17 00:00:00 2001 From: Bernardo Sunderhus Date: Fri, 10 Mar 2023 14:41:31 +0000 Subject: [PATCH 01/19] rfc: slot-children-render-function --- .../slot-children-render-function.md | 530 ++++++++++++++++++ 1 file changed, 530 insertions(+) create mode 100644 rfcs/react-components/convergence/slot-children-render-function.md diff --git a/rfcs/react-components/convergence/slot-children-render-function.md b/rfcs/react-components/convergence/slot-children-render-function.md new file mode 100644 index 0000000000000..02bcaed4d9e7f --- /dev/null +++ b/rfcs/react-components/convergence/slot-children-render-function.md @@ -0,0 +1,530 @@ +# RFC: Slot children render function + +--- + +_@bsunderhus @ling1726 @layershifter_ + +## TL;DR + +Slot children render function is a complex API that is not properly supported. To support this the best alternative so far, +is to create a custom JSX pragma to ensure slot declaration will not lose any property +and will be capable of properly rendering with children render function. + +## Summary + +By our [documentation](https://react.fluentui.dev/?path=/docs/concepts-developer-customizing-components-with-slots--page#replacing-the-entire-slot): + +When you pass content or props to a slot, +the component renders the content within a component or element based on the slot type. +If you need to replace the slot's entire content, including the containing element, +pass a render function as the children. + +This is an escape hatch in the slots API, so prefer the other techniques whenever possible. +If you replace the entire slot, very accessibility, layout, and styling still work properly. + +By passing `renderBigLetterIcon` as the `children`, the `span` that normally contains the icon is replaced with an `b` (bold). + +```tsx +const renderBigLetterIcon (Component, props) => { + return B; +}; + +; +``` + +## Background + +There are 3 instances of properties that will be provided to a slot in different phases of the slot creation; + +1. External properties (`props`) +2. Internal properties (`defaultProps`) +3. Overrides + +### External properties + +External properties (`props`) are the properties provided by the client using a component with a specific slot. +On the `AccordionHeader` example, the `button` slot external properties may be found as `props.button` inside the component's +implementation. + +`props.button` is called a shorthand and can be either: + +1. External properties of the slot +2. `string`, `number`, `Array` or `JSX element` value that will be provided to slot's `children` +3. `null` or `undefined` + +```ts +export const AccordionHeader: ForwardRefComponent = React.forwardRef((props, ref) => { + props.button; // this is the button shorthand +}); +``` + +### Internal properties + +Internal properties (`defaultProps`) are the properties provided by the `useState` hook, that should be overridden by +External properties, those are the basic properties that ensure proper functioning of a slot but allowing the user +to opt out of them. + +They can be declared when resolving the provided shorthand of a slot (`props.button`, in this example): + +```ts +// useAccordionHeader +const buttonShorthand = useARIAButtonShorthand(props.button, { + required: true, + // internal properties + defaultProps: { + disabled, + disabledFocusable, + 'aria-expanded': open, + type: 'button', + }, +}); +``` + +### Overrides + +Overrides are the properties provided by the `render` method, that should override all the other properties. +On most of the implementations of slots we have so far in v9 we tend to avoid using overrides, except for one case that is +spread all over the components, which is `children` override: + +```tsx +// renderAccordionHeader + + + + {state.expandIconPosition === 'start' && } + {slots.icon && } + {slotProps.root.children} + {state.expandIconPosition === 'end' && } + + +``` + +In this example we provide to both `root` and `button` slots overrides of their children value, ignoring +any previous values that was passed either by Internal properties or External properties. Slots that tend to override +their children are normally referred to as `wrappers` since they're normally used for the case of +wrapping other slots inside of them. + +## Current issues + +[Slot children render function won't override existing children. #27089](https://github.com/microsoft/fluentui/issues/27089) + +Repro: https://codesandbox.io/s/elated-babbage-46ez1w?file=/example.tsx + +It's not possible to use children render function to override an entire slot, if `children` are already used. The above is a minimal repro of an issue that can be seen in Fluent in the `AccordionHeader` component. The component renders the `button` slot which contains its own children. JSX children will win over props children - which is how children render functions are rendered by `getSlots` + +https://github.com/microsoft/fluentui/blob/220b321042b4bcc126c86a91811a34b04934d49b/packages/react-components/react-accordion/src/components/AccordionHeader/renderAccordionHeader.tsx#L13-L25 + +Requirements for children render function: + +- should be able to override the entire slot +- When rendering the original children, should respect nested slots. + +## Problem statement + +The main problem revolves around coalescing those 3 instances of properties a slot might have! At the moment, +our current slot declaration mechanism revolves around 2 separate methods used in 2 different moments: + +1. `resolveShorthand` +2. `getSlots` + +`resolveShorthand` is invoked at `useState` hook, it will receive a shorthand and will convert it to either `undefined` or an object comprising +the merging of the External properties and Internal ones, following given priorities (`props` wins over `defaultProps`). + +`getSlots` is invoked at `render` method and will go over every single slot declared on `state.components` and convert every resolved shorthand into a pair of +element to be render and properties to be passed. In this `getSlots` method, we also ensure that if a given resolved shorthand `children` +property is a function, than we'll invoke that function: + +```ts +// getSlots + +if (typeof children === 'function') { + const render = children; + // in this case the pair of element to be render and properties to be passed are converted into + // a Fragment and {children} where `children` will be the return of invoking the `children` render method + return [ + React.Fragment, + { + children: render(slot, propsWithoutAsAndChildren), + } as unknown as R[K], + ]; +} +``` + +### Problem 1 + +The `slot` argument will represent the merging between `state.component.button` value +(back into the `AccordionHeader` `button` slot example) and `props.button.as` value. `propsWithoutAsAndChildren` +as the name suggests is all the properties provided by the `resolveShorthand` invocation without including `as` and also `children` +(since `children` by itself is the render function). + +Since we lost the Internal properties on the merging between it and the External ones +There's no way to provide such the default declared `children` to the render function! + +### Problem 2 + +At `render` method after `getSlots` invocation we have all `slots` and `slotProps` well defined. For the case of wrappers +we'll also provide an override for `children`, taking precedence over External properties and also Internal properties. +This will cause us to lose any children provided by a `render` function passed as a children. + +### Sum up + +At the moment the merging of the 3 instances of properties is done in 3 different steps: + +1. `resolveShorthand` will merge External properties with Internal properties (external takes precedence) +2. `getSlots` will **wrongly** filter properties on case of render function as `children` +3. on slot rendering of the `render` method the merged properties provided by + `getSlots` will be merged with the Overrides (overrides takes precedence), + once again overriding `children` on the case of `wrappers` + + ```tsx + // the third step is the render of the slot itself + + ``` + +Properties will be lost after those steps, mainly `children` will be lost, affecting the render function mechanism. + +Merging the properties is a complex scenario that should not be splitted into multiple steps as all properties +must be considered when merging them, since we got peculiar scenarios involving `children` mostly + +## Detailed Design or Proposal + +### Option A: Custom JSX Pragma + +This problem can be solved by not prematurely merging the 3 instances of properties provided to a slot. To do that +we should find a way to delay as long as possible that merging mechanism. + +The last provided properties are the Overrides, which are provided on rendering time, when a jsx element is presented: + +```tsx +// slotProps and children are the overrides here +Children Override +``` + +Since we only have access to the Overrides on the declaration of the jsx element itself, the only way to postpone +merging of the properties after we have the Overrides is by changing the intrinsic mechanism of how a jsx element is consumed. + +Instead of having a `resolveShorthand` that will return a merge between External properties and Internal properties, +we can provide a custom exotic component, similar to what happens when `React.memo` or `React.forwardRef` does. + +#### slot method over resolveShorthand + +The `slot` method will return a slot component, similar to a component declaration when using `React.memo` or `React.forwardRef`, +and in that slot component declaration, both External properties and Internal properties will coexist without merging, +together with the `componentType` that will work as `state.components.slot` would. + +This will be very similar to the previous usage of `resolveShorthand`: + +```tsx +const state = { + root: slot(getNativeElementProps(as || 'div', { ref, ...props }), { + required: true, + componentType: 'div', + }), + icon: slot(icon, { componentType: 'div' }), + expandIcon: slot(expandIcon, { + required: true, + componentType: 'span', + defaultProps: { + children: , + 'aria-hidden': true, + }, + }), + button: slot(button, { + required: true, + componentType: 'button', + defaultProps: { + disabled, + disabledFocusable, + 'aria-expanded': open, + type: 'button', + }, + }), +}; +``` + +#### render methods + +Since there'll be a custom pragma, there's no need for `getSlots` anymore, the components provided by the `slot` method will +be renderable, meaning we can simply use them in the render! + +```tsx +export const renderAccordionHeader_unstable = (state: AccordionHeaderState) => ( + + + {state.expandIconPosition === 'start' && state.expandIcon && } + {state.icon && } + {state.root.props.children} + {state.expandIconPosition === 'end' && state.expandIcon && } + + +); +``` + +#### Types + +Some type changes will be required: + +##### Introduction of the `SlotComponent` type. + +Slot components are exotic components (like `React.forwardRef` and `React.memo` components), +that will have intrinsically associated with them External properties, Internal properties and the base component type. + +```ts +type SlotComponent = React.ExoticComponent< + Props & React.RefAttributes +> & { + readonly props: Props; + readonly defaultProps?: Partial | undefined; + readonly componentType: + | React.ComponentType + | (Props extends AsIntrinsicElement ? As : keyof JSX.IntrinsicElements); +}; +``` + +##### ComponentState + +`ComponentState` should map provided slots to `SlotComponent`s instead of mapping to resolved shorthands! And `components` property can be dropped. + +Before: + +```ts +type ComponentState = { + components: { + [Key in keyof Slots]-?: + | React.ComponentType> + | (ExtractSlotProps extends AsIntrinsicElement ? As : keyof JSX.IntrinsicElements); + }; +} & { + // Include a prop for each slot, with the shorthand resolved to a props object + // The root slot can never be null, so also exclude null from it + [Key in keyof Slots]: ReplaceNullWithUndefined< + Exclude + >; +}; +``` + +After: + +```ts +type ComponentState = { + [Key in keyof Slots]: SlotComponent>; +}; +``` + +#### Styles hooks + +A minor modification in style hooks are required. at the moment we mutate the merged properties provided by `resolveShorthand` +to include our custom `className` property. + +In this case we have some alternatives: + +##### Option 1 mutate external properties + +Instead of mutating the merged properties, simply mutate the External properties from the slot component: + +Before: + +```ts +// useAccordionHeader +export const useAccordionHeaderStyles_unstable = (state: AccordionHeaderState) => { + const styles = useStyles(); + state.root.className = mergeClasses( + accordionHeaderClassNames.root, + styles.root, + state.inline && styles.rootInline, + state.disabled && styles.rootDisabled, + state.root.className, + ); + + return state; +}; +``` + +After: + +```ts +// useAccordionHeader +export const useAccordionHeaderStyles_unstable = (state: AccordionHeaderState) => { + const styles = useStyles(); + state.root.props.className = mergeClasses( + accordionHeaderClassNames.root, + styles.root, + state.inline && styles.rootInline, + state.disabled && styles.rootDisabled, + state.root.props.className, + ); + + return state; +}; +``` + +##### Option 2 mutate overrides + +By introducing an `overrides` property on the `ComponentState` we can create a layer of override that can me mutate + +After: + +```tsx +export const useAccordionHeaderStyles_unstable = (state: AccordionHeaderState) => { + const styles = useStyles(); + state.overrides.root.className = mergeClasses( + accordionHeaderClassNames.root, + styles.root, + state.inline && styles.rootInline, + state.disabled && styles.rootDisabled, + state.root.props.className, + ); + + return state; +}; + +export const renderAccordionHeader_unstable = (state: AccordionHeaderState) => ( + + + {state.expandIconPosition === 'start' && state.expandIcon && } + {state.icon && } + {state.root.props.children} + {state.expandIconPosition === 'end' && state.expandIcon && } + + +); +``` + +##### Option 3 stop mutating + +Let's just stop mutating, and since we require a layer of custom styling through classNames +let's just properly use an argument on the render method exclusively for this: + +After: + +```tsx +export const AccordionHeader: ForwardRefComponent = React.forwardRef((props, ref) => { + const state = useAccordionHeader_unstable(props, ref); + const styles = useAccordionHeaderStyles_unstable(state); + const contextValues = useAccordionHeaderContextValues_unstable(state); + return renderAccordionHeader_unstable(state, styles, contextValues); +}); + +export const useAccordionHeaderStyles_unstable = (state: AccordionHeaderState): AccordionHeaderStyles => { + const styles = useStyles(); + return { + root: mergeClasses( + accordionHeaderClassNames.root, + styles.root, + state.inline && styles.rootInline, + state.disabled && styles.rootDisabled, + state.root.props.className, + ), + }; +}; + +export const renderAccordionHeader_unstable = (state: AccordionHeaderState, styles: AccordionHeaderStyles) => ( + + + {state.expandIconPosition === 'start' && state.expandIcon && } + {state.icon && } + {state.root.props.children} + {state.expandIconPosition === 'end' && state.expandIcon && } + + +); +``` + +#### Passing overrides from state to render + +Since our logic lies on the state hooks, the override methods will be implemented on those state hooks, although it should be +declared on the render method explicitly to properly work as an override. + +For example, on the case `AccordionHeader` the `button` slot would override the `onClick` method. For that a property called `overrides` +can be explicitly added to `AccordionHeaderState` stating such override: + +```tsx +// AccordionHeader.types.ts +type AccordionHeaderState = NextComponentState & { + overrides: { + button: { + onClick(ev: React.MouseEvent): void; + }; + }; +}; + +// useAccordionHeader.ts +const state = { + // ... + overrides: { + button: { + onClick: useEventCallback(ev => { + buttonSlot.props.onClick?.(ev); + if (!ev.defaultPrevented) { + onAccordionHeaderClick(ev); + } + }), + }, + }, +}; + +// renderAccordionHeader.tsx +const renderAccordionHeader_unstable = (state: AccordionHeaderState) => ( + + + {state.expandIconPosition === 'start' && state.expandIcon && } + {state.icon && } + {state.root.props.children} + {state.expandIconPosition === 'end' && state.expandIcon && } + + +); +``` + +#### Custom Pragma implementation + +The custom pragma implementation will have the responsibility of acting similar to what `getSlots` used to do, +but without having the 3 instances of properties being merged, +which will allow for it to properly handle all the edge case scenarios that comes with our current API + +This is a first draft of it, it is pretty similar to what is being done internally by `getSlot` method, +but returning an element instead of a tuple: + +```ts +function jsxFromSlotComponent( + component: SlotComponent, + overrides?: Props | null, + ...childrenOverride: React.ReactNode[] +): React.ReactElement | null { + const props = { ...component.defaultProps, ...component.props, ...overrides }; + const children = normalizeChildren(component.props, component.defaultProps, overrides, ...childrenOverride); + const { as: asProp, ...propsWithoutAs } = props; + + const elementType = + component.componentType === undefined || typeof component.componentType === 'string' + ? asProp ?? component.componentType ?? 'div' + : component.componentType; + + // on the case of an External property of children as render then this overrides even the override children. + if (typeof component.props.children === 'function') { + const render = component.props.children; + return React.createElement( + React.Fragment, + {}, + // children will not be lost in this case! + render(elementType, { ...propsWithoutAs, children }), + ) as React.ReactElement; + } + + const shouldOmitAsProp = typeof elementType === 'string' && asProp; + + return React.createElement(elementType, shouldOmitAsProp ? propsWithoutAs : props, children); +} +``` + +## Pros and Cons + +### Pros + +1. Simple to use +2. Less code than what we currently have +3. No iteration over `state.components` is required to define slots + +### Cons + +1. Requires extra build steps From c2b1e8c5dcbdc7ce2f9b800dcdc7421db239b8c3 Mon Sep 17 00:00:00 2001 From: Bernardo Sunderhus Date: Mon, 13 Mar 2023 13:27:38 +0000 Subject: [PATCH 02/19] chore: follow up on Hotell comments --- .../slot-children-render-function.md | 102 +++++++++--------- 1 file changed, 49 insertions(+), 53 deletions(-) diff --git a/rfcs/react-components/convergence/slot-children-render-function.md b/rfcs/react-components/convergence/slot-children-render-function.md index 02bcaed4d9e7f..5d79938b856f7 100644 --- a/rfcs/react-components/convergence/slot-children-render-function.md +++ b/rfcs/react-components/convergence/slot-children-render-function.md @@ -14,15 +14,11 @@ and will be capable of properly rendering with children render function. By our [documentation](https://react.fluentui.dev/?path=/docs/concepts-developer-customizing-components-with-slots--page#replacing-the-entire-slot): -When you pass content or props to a slot, -the component renders the content within a component or element based on the slot type. -If you need to replace the slot's entire content, including the containing element, -pass a render function as the children. - -This is an escape hatch in the slots API, so prefer the other techniques whenever possible. -If you replace the entire slot, very accessibility, layout, and styling still work properly. - -By passing `renderBigLetterIcon` as the `children`, the `span` that normally contains the icon is replaced with an `b` (bold). +> When you pass content or props to a slot, the component renders the content within a component or element based on the slot type. +> If you need to replace the slot's entire content, including the containing element, pass a render function as the children. +> This is an escape hatch in the slots API, so prefer the other techniques whenever possible. +> If you replace the entire slot, accessibility, layout, and styling still work properly. +> By passing `renderBigLetterIcon` as the `children`, the `span` that normally contains the icon is replaced with an `b` (bold). ```tsx const renderBigLetterIcon (Component, props) => { @@ -322,62 +318,49 @@ In this case we have some alternatives: Instead of mutating the merged properties, simply mutate the External properties from the slot component: -Before: - -```ts +```diff // useAccordionHeader export const useAccordionHeaderStyles_unstable = (state: AccordionHeaderState) => { const styles = useStyles(); - state.root.className = mergeClasses( +- state.root.className = mergeClasses( ++ state.root.props.className = mergeClasses( accordionHeaderClassNames.root, styles.root, state.inline && styles.rootInline, state.disabled && styles.rootDisabled, - state.root.className, +- state.root.className, ++ state.root.props.className, ); return state; }; ``` -After: +##### Option 2 mutate overrides -```ts +By introducing an `overrides` property on the `ComponentState` we can create a layer of override that can be mutated: + +```diff // useAccordionHeader export const useAccordionHeaderStyles_unstable = (state: AccordionHeaderState) => { const styles = useStyles(); - state.root.props.className = mergeClasses( +- state.root.className = mergeClasses( ++ state.overrides.root.className = mergeClasses( accordionHeaderClassNames.root, styles.root, state.inline && styles.rootInline, state.disabled && styles.rootDisabled, - state.root.props.className, +- state.root.className, ++ state.root.props.className, ); return state; }; ``` -##### Option 2 mutate overrides - -By introducing an `overrides` property on the `ComponentState` we can create a layer of override that can me mutate - -After: +On the render function side, we just need to spread the overrides: ```tsx -export const useAccordionHeaderStyles_unstable = (state: AccordionHeaderState) => { - const styles = useStyles(); - state.overrides.root.className = mergeClasses( - accordionHeaderClassNames.root, - styles.root, - state.inline && styles.rootInline, - state.disabled && styles.rootDisabled, - state.root.props.className, - ); - - return state; -}; - export const renderAccordionHeader_unstable = (state: AccordionHeaderState) => ( @@ -395,31 +378,42 @@ export const renderAccordionHeader_unstable = (state: AccordionHeaderState) => ( Let's just stop mutating, and since we require a layer of custom styling through classNames let's just properly use an argument on the render method exclusively for this: -After: - -```tsx -export const AccordionHeader: ForwardRefComponent = React.forwardRef((props, ref) => { - const state = useAccordionHeader_unstable(props, ref); - const styles = useAccordionHeaderStyles_unstable(state); - const contextValues = useAccordionHeaderContextValues_unstable(state); - return renderAccordionHeader_unstable(state, styles, contextValues); -}); +Before: -export const useAccordionHeaderStyles_unstable = (state: AccordionHeaderState): AccordionHeaderStyles => { +```diff +// useAccordionHeader +-export const useAccordionHeaderStyles_unstable = (state: AccordionHeaderState) => { ++export const useAccordionHeaderStyles_unstable = (state: AccordionHeaderState): AccordionHeaderStyles => { const styles = useStyles(); - return { - root: mergeClasses( ++ return { +- state.root.className = mergeClasses( ++ root: mergeClasses( accordionHeaderClassNames.root, styles.root, state.inline && styles.rootInline, state.disabled && styles.rootDisabled, - state.root.props.className, - ), - }; +- state.root.className, ++ state.root.props.className, + ); ++ } +- return state; }; -export const renderAccordionHeader_unstable = (state: AccordionHeaderState, styles: AccordionHeaderStyles) => ( - +// AccordionHeader +export const AccordionHeader: ForwardRefComponent = React.forwardRef((props, ref) => { + const state = useAccordionHeader_unstable(props, ref); +- useAccordionHeaderStyles_unstable(state); ++ const styles = useAccordionHeaderStyles_unstable(state); + const contextValues = useAccordionHeaderContextValues_unstable(state); +- return renderAccordionHeader_unstable(state, contextValues); ++ return renderAccordionHeader_unstable(state, styles, contextValues); +}); + +// renderAccordionHeader +- export const renderAccordionHeader_unstable = (state: AccordionHeaderState) => ( ++ export const renderAccordionHeader_unstable = (state: AccordionHeaderState, styles: AccordionHeaderStyles) => ( +- ++ {state.expandIconPosition === 'start' && state.expandIcon && } {state.icon && } @@ -524,6 +518,8 @@ function jsxFromSlotComponent( 1. Simple to use 2. Less code than what we currently have 3. No iteration over `state.components` is required to define slots +4. Will simplify the whole underlying architecture +5. No Breaking changes, the consumers will not have any impact (besides allowing the children render function to work properly) ### Cons From 2fc22de9621d3d78a41712652add1f5bf9410f4e Mon Sep 17 00:00:00 2001 From: Bernardo Sunderhus Date: Wed, 15 Mar 2023 07:44:18 -0300 Subject: [PATCH 03/19] Update rfcs/react-components/convergence/slot-children-render-function.md Co-authored-by: Oleksandr Fediashov --- .../convergence/slot-children-render-function.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rfcs/react-components/convergence/slot-children-render-function.md b/rfcs/react-components/convergence/slot-children-render-function.md index 5d79938b856f7..2edb71736111b 100644 --- a/rfcs/react-components/convergence/slot-children-render-function.md +++ b/rfcs/react-components/convergence/slot-children-render-function.md @@ -112,7 +112,7 @@ https://github.com/microsoft/fluentui/blob/220b321042b4bcc126c86a91811a34b04934d Requirements for children render function: -- should be able to override the entire slot +- Should be able to override the entire slot. - When rendering the original children, should respect nested slots. ## Problem statement From 0136d4ced39a6c6500a536b5259582a6ee595b12 Mon Sep 17 00:00:00 2001 From: Bernardo Sunderhus Date: Wed, 15 Mar 2023 07:44:30 -0300 Subject: [PATCH 04/19] Update rfcs/react-components/convergence/slot-children-render-function.md Co-authored-by: Oleksandr Fediashov --- .../convergence/slot-children-render-function.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rfcs/react-components/convergence/slot-children-render-function.md b/rfcs/react-components/convergence/slot-children-render-function.md index 2edb71736111b..d812826127bad 100644 --- a/rfcs/react-components/convergence/slot-children-render-function.md +++ b/rfcs/react-components/convergence/slot-children-render-function.md @@ -30,7 +30,7 @@ const renderBigLetterIcon (Component, props) => { ## Background -There are 3 instances of properties that will be provided to a slot in different phases of the slot creation; +There are 3 instances of properties that will be provided to a slot in different phases of the slot creation: 1. External properties (`props`) 2. Internal properties (`defaultProps`) From 16299a02bd1c69cc1d7c4262d1a86ac70f2f9582 Mon Sep 17 00:00:00 2001 From: Bernardo Sunderhus Date: Wed, 15 Mar 2023 07:44:57 -0300 Subject: [PATCH 05/19] Update rfcs/react-components/convergence/slot-children-render-function.md Co-authored-by: Oleksandr Fediashov --- .../convergence/slot-children-render-function.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rfcs/react-components/convergence/slot-children-render-function.md b/rfcs/react-components/convergence/slot-children-render-function.md index d812826127bad..03e22d5a36ae3 100644 --- a/rfcs/react-components/convergence/slot-children-render-function.md +++ b/rfcs/react-components/convergence/slot-children-render-function.md @@ -44,7 +44,7 @@ implementation. `props.button` is called a shorthand and can be either: -1. External properties of the slot +1. External properties of the slot (an object) 2. `string`, `number`, `Array` or `JSX element` value that will be provided to slot's `children` 3. `null` or `undefined` From 465dffd500391d1336bb4b85efa45ccb231a7eec Mon Sep 17 00:00:00 2001 From: Bernardo Sunderhus Date: Wed, 15 Mar 2023 17:08:17 +0000 Subject: [PATCH 06/19] chore: add option --- .../slot-children-render-function.md | 45 +++++++++++++++++-- 1 file changed, 42 insertions(+), 3 deletions(-) diff --git a/rfcs/react-components/convergence/slot-children-render-function.md b/rfcs/react-components/convergence/slot-children-render-function.md index 03e22d5a36ae3..946786387609a 100644 --- a/rfcs/react-components/convergence/slot-children-render-function.md +++ b/rfcs/react-components/convergence/slot-children-render-function.md @@ -511,9 +511,9 @@ function jsxFromSlotComponent( } ``` -## Pros and Cons +#### Pros and Cons -### Pros +##### Pros 1. Simple to use 2. Less code than what we currently have @@ -521,6 +521,45 @@ function jsxFromSlotComponent( 4. Will simplify the whole underlying architecture 5. No Breaking changes, the consumers will not have any impact (besides allowing the children render function to work properly) -### Cons +#### Cons 1. Requires extra build steps + +### Option B: Refactor getSlots + helper method + +An alternative solution would be to stop children function rendering on `getSlots` method (since we don't have enough information at that moment) +and then wrapping all children overrides with a helper method (let's call it `resolveChildren`): + +```tsx +// renderAccordionHeader + + {resolveChildren( + slotProps.root.children, + + {resolveChildren( + slotProps.button.children, + <> + {state.expandIconPosition === 'start' && ( + + {resolveChildren( + slotProps.expandIcon.children, + , + )} + + )} + {slots.icon && } + {typeof slotProps.root.children !== 'function' && slotProps.root.children} + {state.expandIconPosition === 'end' && ( + + {resolveChildren( + slotProps.expandIcon.children, + , + )} + + )} + , + )} + , + )} + +``` From 2be637ff199c7961657aab75397254de4a11f6a0 Mon Sep 17 00:00:00 2001 From: Bernardo Sunderhus Date: Thu, 16 Mar 2023 11:29:19 +0000 Subject: [PATCH 07/19] chore: adds option C --- .../slot-children-render-function.md | 51 ++++++++++++++++--- 1 file changed, 43 insertions(+), 8 deletions(-) diff --git a/rfcs/react-components/convergence/slot-children-render-function.md b/rfcs/react-components/convergence/slot-children-render-function.md index 946786387609a..c22212bc39e6b 100644 --- a/rfcs/react-components/convergence/slot-children-render-function.md +++ b/rfcs/react-components/convergence/slot-children-render-function.md @@ -516,14 +516,16 @@ function jsxFromSlotComponent( ##### Pros 1. Simple to use -2. Less code than what we currently have -3. No iteration over `state.components` is required to define slots +2. Reduces codebase size (`getSlots` invocations can be stripped) +3. No iteration over `state.components` is required to define slots (the slots used are the slots being processed) 4. Will simplify the whole underlying architecture 5. No Breaking changes, the consumers will not have any impact (besides allowing the children render function to work properly) +6. couples together `state.components` with the slot itself -#### Cons +##### Cons 1. Requires extra build steps +2. Requires a small migration (but it can be done granularly, nothing will break) ### Option B: Refactor getSlots + helper method @@ -534,25 +536,25 @@ and then wrapping all children overrides with a helper method (let's call it `re // renderAccordionHeader {resolveChildren( - slotProps.root.children, + slotProps.root, {resolveChildren( - slotProps.button.children, + slotProps.button, <> {state.expandIconPosition === 'start' && ( {resolveChildren( - slotProps.expandIcon.children, + slotProps.expandIcon, , )} )} {slots.icon && } - {typeof slotProps.root.children !== 'function' && slotProps.root.children} + {slotProps.root.children} {state.expandIconPosition === 'end' && ( {resolveChildren( - slotProps.expandIcon.children, + slotProps.expandIcon, , )} @@ -563,3 +565,36 @@ and then wrapping all children overrides with a helper method (let's call it `re )} ``` + +#### Pros and Cons + +##### Pros + +1. It would solve the problem?! + +##### Cons + +1. Render methods will become more complex + +### Option C: Custom JSX Pragma without internal changes + +A minimal version of option A, but without introducing internal changes. If we focus on the problem itself, +slot children render function, the main requirement would be to maintain `defaultProps.children` until the last possible +moment so that it can be introduced once again back to a children render function. + +This proposal would take advantage of `Symbol` to introduce a "private" method to the result of `resolveShorthand`, +to maintain the value of `defaultProps.children`. That value would then later on be consumed by a minimal custom JSX pragma +that on the specific case of children render function this pragma would consume this `defaultProps.children` to ensure proper functionality. + +https://codesandbox.io/s/wispy-leftpad-f8yi37?file=/example.tsx. + +#### Pros and Cons + +##### Pros + +1. No internal changes required +2. No migration is required + +##### Cons + +1. Requires extra build step From 87f0be645ae664497e0b706bfe9ad1e7ad96c79f Mon Sep 17 00:00:00 2001 From: Bernardo Sunderhus Date: Fri, 17 Mar 2023 11:55:52 +0000 Subject: [PATCH 08/19] chore: adds option D --- .../slot-children-render-function.md | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/rfcs/react-components/convergence/slot-children-render-function.md b/rfcs/react-components/convergence/slot-children-render-function.md index c22212bc39e6b..c34fc3d6a250b 100644 --- a/rfcs/react-components/convergence/slot-children-render-function.md +++ b/rfcs/react-components/convergence/slot-children-render-function.md @@ -586,6 +586,20 @@ This proposal would take advantage of `Symbol` to introduce a "private" method t to maintain the value of `defaultProps.children`. That value would then later on be consumed by a minimal custom JSX pragma that on the specific case of children render function this pragma would consume this `defaultProps.children` to ensure proper functionality. +```diff +const defaultPropsChildrenSymbol = Symbol('fuiSlotDefaultPropsChildren') + +export const resolveShorthand: ResolveShorthandFunction = (value, options) => { + //... + + return defaultProps ? { + ...defaultProps, + ...resolvedShorthand, ++ [defaultPropsChildrenSymbol]: defaultProps?.children + } : resolvedShorthand; +}; +``` + https://codesandbox.io/s/wispy-leftpad-f8yi37?file=/example.tsx. #### Pros and Cons @@ -598,3 +612,39 @@ https://codesandbox.io/s/wispy-leftpad-f8yi37?file=/example.tsx. ##### Cons 1. Requires extra build step + +### Option D: Option A + Option C + +Taking the strategy provided on Option C to hide required data with `Symbol` inside the result value of `resolveShorthand` +and applying it to not only move forward the `defaultProps.children` (from Option C) but also `componentType` (from Option A) +we can completely get rid of `getSlots` invocation, as all the data required to render a slot will be provided by `resolveShorthand` and only consumed at render by a custom pragma. + +```diff +const defaultPropsChildrenSymbol = Symbol('fuiSlotDefaultPropsChildren') +const componentTypeSymbol = Symbol('fuiSlotComponentType') + +export const resolveShorthand: ResolveShorthandFunction = (value, options) => { + //... + + return defaultProps ? { + ...defaultProps, + ...resolvedShorthand, ++ [defaultPropsChildrenSymbol]: defaultProps?.children ++ [componentTypeSymbol]: options.componentType + } : resolvedShorthand; +}; +``` + +##### Pros + +1. Simple to use +2. Reduces codebase size (`getSlots` invocations can be stripped) +3. No iteration over `state.components` is required to define slots (the slots used are the slots being processed) +4. Will simplify the whole underlying architecture +5. No Breaking changes, the consumers will not have any impact (besides allowing the children render function to work properly) +6. couples together `state.components` with the slot itself + +##### Cons + +1. Requires extra build steps +2. Requires to remove `getSlots` usage (but it can be done granularly, nothing will break) From 202ca13d1189f6908b5b37313bdb750ba7e3c7ab Mon Sep 17 00:00:00 2001 From: Bernardo Sunderhus Date: Wed, 22 Mar 2023 09:41:53 -0300 Subject: [PATCH 09/19] Update rfcs/react-components/convergence/slot-children-render-function.md Co-authored-by: Oleksandr Fediashov --- .../convergence/slot-children-render-function.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/rfcs/react-components/convergence/slot-children-render-function.md b/rfcs/react-components/convergence/slot-children-render-function.md index c34fc3d6a250b..6be357c4b5c44 100644 --- a/rfcs/react-components/convergence/slot-children-render-function.md +++ b/rfcs/react-components/convergence/slot-children-render-function.md @@ -156,6 +156,19 @@ as the name suggests is all the properties provided by the `resolveShorthand` in Since we lost the Internal properties on the merging between it and the External ones There's no way to provide such the default declared `children` to the render function! +```js +function useComponent(props) { + return { + slot: resolveShorthand(props.slot, { children: 'foo' }) + } +} + +const state = useComponent({ children: () => {} }) +typeof state.slot.children === 'function' // true + +// 💥 We lost `defaultProps.children` i.e. `children: "foo"` +``` + ### Problem 2 At `render` method after `getSlots` invocation we have all `slots` and `slotProps` well defined. For the case of wrappers From 5a9488e15d2d0a25a4e8d0ab0d49807e9ca9701c Mon Sep 17 00:00:00 2001 From: Bernardo Sunderhus Date: Wed, 22 Mar 2023 09:42:06 -0300 Subject: [PATCH 10/19] Update rfcs/react-components/convergence/slot-children-render-function.md Co-authored-by: Oleksandr Fediashov --- .../convergence/slot-children-render-function.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/rfcs/react-components/convergence/slot-children-render-function.md b/rfcs/react-components/convergence/slot-children-render-function.md index 6be357c4b5c44..5b0641e60dac8 100644 --- a/rfcs/react-components/convergence/slot-children-render-function.md +++ b/rfcs/react-components/convergence/slot-children-render-function.md @@ -175,6 +175,21 @@ At `render` method after `getSlots` invocation we have all `slots` and `slotProp we'll also provide an override for `children`, taking precedence over External properties and also Internal properties. This will cause us to lose any children provided by a `render` function passed as a children. +```js +function renderComponent(state) { + const { slots, slotProps } = getSlots(state) + + return Foo +} + +const template = renderComponent({ root: { children: 'Baz' }}) + +template.props.children === 'Baz' // false +template.props.children === 'Foo' // true + +// 💥 "Foo" wins over input passed by a user +``` + ### Sum up At the moment the merging of the 3 instances of properties is done in 3 different steps: From 8d453d32b579df37dadd642ddc85a792071381eb Mon Sep 17 00:00:00 2001 From: Bernardo Sunderhus Date: Wed, 22 Mar 2023 09:43:06 -0300 Subject: [PATCH 11/19] Update rfcs/react-components/convergence/slot-children-render-function.md Co-authored-by: Oleksandr Fediashov --- .../convergence/slot-children-render-function.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rfcs/react-components/convergence/slot-children-render-function.md b/rfcs/react-components/convergence/slot-children-render-function.md index 5b0641e60dac8..29e54ade846ba 100644 --- a/rfcs/react-components/convergence/slot-children-render-function.md +++ b/rfcs/react-components/convergence/slot-children-render-function.md @@ -7,7 +7,7 @@ _@bsunderhus @ling1726 @layershifter_ ## TL;DR Slot children render function is a complex API that is not properly supported. To support this the best alternative so far, -is to create a custom JSX pragma to ensure slot declaration will not lose any property +is to create a custom [JSX pragma](https://www.gatsbyjs.com/blog/2019-08-02-what-is-jsx-pragma/) to ensure slot declaration will not lose any property and will be capable of properly rendering with children render function. ## Summary From 117ea87b3461d5b18ac47fcb450fc314666e9e72 Mon Sep 17 00:00:00 2001 From: Bernardo Sunderhus Date: Wed, 22 Mar 2023 09:43:22 -0300 Subject: [PATCH 12/19] Update rfcs/react-components/convergence/slot-children-render-function.md Co-authored-by: Oleksandr Fediashov --- .../convergence/slot-children-render-function.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rfcs/react-components/convergence/slot-children-render-function.md b/rfcs/react-components/convergence/slot-children-render-function.md index 29e54ade846ba..4336ac66c6291 100644 --- a/rfcs/react-components/convergence/slot-children-render-function.md +++ b/rfcs/react-components/convergence/slot-children-render-function.md @@ -56,7 +56,7 @@ export const AccordionHeader: ForwardRefComponent = React. ### Internal properties -Internal properties (`defaultProps`) are the properties provided by the `useState` hook, that should be overridden by +Internal properties (`defaultProps`) are the properties provided by the `use_COMPONENT_` hook, that should be overridden by External properties, those are the basic properties that ensure proper functioning of a slot but allowing the user to opt out of them. From 3943206678b88220ac271a2eee75e4740330888f Mon Sep 17 00:00:00 2001 From: Bernardo Sunderhus Date: Wed, 22 Mar 2023 09:43:35 -0300 Subject: [PATCH 13/19] Update rfcs/react-components/convergence/slot-children-render-function.md Co-authored-by: Oleksandr Fediashov --- .../convergence/slot-children-render-function.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/rfcs/react-components/convergence/slot-children-render-function.md b/rfcs/react-components/convergence/slot-children-render-function.md index 4336ac66c6291..d402427ad5ea0 100644 --- a/rfcs/react-components/convergence/slot-children-render-function.md +++ b/rfcs/react-components/convergence/slot-children-render-function.md @@ -68,8 +68,6 @@ const buttonShorthand = useARIAButtonShorthand(props.button, { required: true, // internal properties defaultProps: { - disabled, - disabledFocusable, 'aria-expanded': open, type: 'button', }, From d4c3039bbeee81d34974a5c91d155dfe188434cb Mon Sep 17 00:00:00 2001 From: Bernardo Sunderhus Date: Wed, 22 Mar 2023 09:44:05 -0300 Subject: [PATCH 14/19] Update rfcs/react-components/convergence/slot-children-render-function.md Co-authored-by: Oleksandr Fediashov --- .../slot-children-render-function.md | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/rfcs/react-components/convergence/slot-children-render-function.md b/rfcs/react-components/convergence/slot-children-render-function.md index d402427ad5ea0..54f770e243620 100644 --- a/rfcs/react-components/convergence/slot-children-render-function.md +++ b/rfcs/react-components/convergence/slot-children-render-function.md @@ -98,6 +98,34 @@ any previous values that was passed either by Internal properties or External pr their children are normally referred to as `wrappers` since they're normally used for the case of wrapping other slots inside of them. +#### What happens behind the scenes? + +```js +function render() { + return ( + + {slotProps.root.children} + + ); +} +``` + +Code above will be compiled to following: + +```js +function render() { + return React.createElement( + slots.button, + slotProps.button, + slotProps.root.children + ); +} +``` + +If `children` specified (third argument of `React.createElement()` i.e. `slotProps.root.children`) they will win over `props` (second argument i.e. `slotProps.button`) + +https://github.com/facebook/react/blob/c0b34bc5fb67fd689383b07ef9d8378814348cd5/packages/react/src/ReactElement.js#L404-L406 + ## Current issues [Slot children render function won't override existing children. #27089](https://github.com/microsoft/fluentui/issues/27089) From 090ab480683718a0a7eddcd5877f52d259a85f0a Mon Sep 17 00:00:00 2001 From: Bernardo Sunderhus Date: Wed, 22 Mar 2023 13:00:11 +0000 Subject: [PATCH 15/19] chore: updates RFC --- .../slot-children-render-function.md | 101 ++++++++++++------ 1 file changed, 67 insertions(+), 34 deletions(-) diff --git a/rfcs/react-components/convergence/slot-children-render-function.md b/rfcs/react-components/convergence/slot-children-render-function.md index 54f770e243620..20ff730a65e8b 100644 --- a/rfcs/react-components/convergence/slot-children-render-function.md +++ b/rfcs/react-components/convergence/slot-children-render-function.md @@ -10,7 +10,9 @@ Slot children render function is a complex API that is not properly supported. T is to create a custom [JSX pragma](https://www.gatsbyjs.com/blog/2019-08-02-what-is-jsx-pragma/) to ensure slot declaration will not lose any property and will be capable of properly rendering with children render function. -## Summary +## Background + +--- By our [documentation](https://react.fluentui.dev/?path=/docs/concepts-developer-customizing-components-with-slots--page#replacing-the-entire-slot): @@ -28,7 +30,7 @@ const renderBigLetterIcon (Component, props) => { ; ``` -## Background +--- There are 3 instances of properties that will be provided to a slot in different phases of the slot creation: @@ -102,11 +104,7 @@ wrapping other slots inside of them. ```js function render() { - return ( - - {slotProps.root.children} - - ); + return {slotProps.root.children}; } ``` @@ -114,11 +112,7 @@ Code above will be compiled to following: ```js function render() { - return React.createElement( - slots.button, - slotProps.button, - slotProps.root.children - ); + return React.createElement(slots.button, slotProps.button, slotProps.root.children); } ``` @@ -185,12 +179,12 @@ There's no way to provide such the default declared `children` to the render fun ```js function useComponent(props) { return { - slot: resolveShorthand(props.slot, { children: 'foo' }) - } + slot: resolveShorthand(props.slot, { children: 'foo' }), + }; } -const state = useComponent({ children: () => {} }) -typeof state.slot.children === 'function' // true +const state = useComponent({ children: () => {} }); +typeof state.slot.children === 'function'; // true // 💥 We lost `defaultProps.children` i.e. `children: "foo"` ``` @@ -203,15 +197,15 @@ This will cause us to lose any children provided by a `render` function passed a ```js function renderComponent(state) { - const { slots, slotProps } = getSlots(state) + const { slots, slotProps } = getSlots(state); - return Foo + return Foo; } -const template = renderComponent({ root: { children: 'Baz' }}) +const template = renderComponent({ root: { children: 'Baz' } }); -template.props.children === 'Baz' // false -template.props.children === 'Foo' // true +template.props.children === 'Baz'; // false +template.props.children === 'Foo'; // true // 💥 "Foo" wins over input passed by a user ``` @@ -386,6 +380,16 @@ export const useAccordionHeaderStyles_unstable = (state: AccordionHeaderState) = + state.root.props.className, ); + if (state.icon) { +- state.icon.className = mergeClasses( ++ state.icon.props.className = mergeClasses( + accordionHeaderClassNames.icon, + styles.icon, +- state.icon.className ++ state.icon.props.className + ); + } + return state; }; ``` @@ -408,6 +412,16 @@ export const useAccordionHeaderStyles_unstable = (state: AccordionHeaderState) = + state.root.props.className, ); + if (state.icon) { +- state.icon.className = mergeClasses( ++ state.overrides.icon.className = mergeClasses( + accordionHeaderClassNames.icon, + styles.icon, +- state.icon.className ++ state.icon.props.className + ); + } + return state; }; ``` @@ -439,18 +453,36 @@ Before: -export const useAccordionHeaderStyles_unstable = (state: AccordionHeaderState) => { +export const useAccordionHeaderStyles_unstable = (state: AccordionHeaderState): AccordionHeaderStyles => { const styles = useStyles(); -+ return { - state.root.className = mergeClasses( +- accordionHeaderClassNames.root, +- styles.root, +- state.inline && styles.rootInline, +- state.disabled && styles.rootDisabled, +- state.root.className, +- ); + +- if (state.icon) { +- state.icon.className = mergeClasses( +- accordionHeaderClassNames.icon, +- styles.icon, +- state.icon.className +- ); +- } +- return state; ++ return { + root: mergeClasses( - accordionHeaderClassNames.root, - styles.root, - state.inline && styles.rootInline, - state.disabled && styles.rootDisabled, -- state.root.className, ++ accordionHeaderClassNames.root, ++ styles.root, ++ state.inline && styles.rootInline, ++ state.disabled && styles.rootDisabled, + state.root.props.className, - ); ++ ); ++ icon: state.icon && mergeClasses( ++ accordionHeaderClassNames.icon, ++ styles.icon, ++ state.icon.props.className ++ ) + } -- return state; }; // AccordionHeader @@ -667,7 +699,7 @@ https://codesandbox.io/s/wispy-leftpad-f8yi37?file=/example.tsx. 1. Requires extra build step -### Option D: Option A + Option C +### Option D: Option A (partially) + Option C Taking the strategy provided on Option C to hide required data with `Symbol` inside the result value of `resolveShorthand` and applying it to not only move forward the `defaultProps.children` (from Option C) but also `componentType` (from Option A) @@ -693,10 +725,11 @@ export const resolveShorthand: ResolveShorthandFunction = (value, options) => { 1. Simple to use 2. Reduces codebase size (`getSlots` invocations can be stripped) -3. No iteration over `state.components` is required to define slots (the slots used are the slots being processed) -4. Will simplify the whole underlying architecture -5. No Breaking changes, the consumers will not have any impact (besides allowing the children render function to work properly) -6. couples together `state.components` with the slot itself +3. `state.components` can be stripped +4. No iteration over `state.components` is required to define slots (the slots used are the slots being processed) +5. Will simplify the whole underlying architecture +6. No Breaking changes, the consumers will not have any impact (besides allowing the children render function to work properly) +7. couples together `state.components` with the slot itself ##### Cons From e2a9b129df7dfa865c335cae9983f3e71ef07da8 Mon Sep 17 00:00:00 2001 From: Bernardo Sunderhus Date: Wed, 22 Mar 2023 13:05:59 +0000 Subject: [PATCH 16/19] chore: adds ToC --- .../slot-children-render-function.md | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/rfcs/react-components/convergence/slot-children-render-function.md b/rfcs/react-components/convergence/slot-children-render-function.md index 20ff730a65e8b..fb0c1caea6870 100644 --- a/rfcs/react-components/convergence/slot-children-render-function.md +++ b/rfcs/react-components/convergence/slot-children-render-function.md @@ -4,6 +4,53 @@ _@bsunderhus @ling1726 @layershifter_ + + + +**Table of Contents** + +- [RFC: Slot children render function](#rfc-slot-children-render-function) + - [TL;DR](#tldr) + - [Background](#background) + - [External properties](#external-properties) + - [Internal properties](#internal-properties) + - [Overrides](#overrides) + - [What happens behind the scenes?](#what-happens-behind-the-scenes) + - [Current issues](#current-issues) + - [Problem statement](#problem-statement) + - [Problem 1](#problem-1) + - [Problem 2](#problem-2) + - [Sum up](#sum-up) + - [Detailed Design or Proposal](#detailed-design-or-proposal) + - [Option A: Custom JSX Pragma](#option-a-custom-jsx-pragma) + - [slot method over resolveShorthand](#slot-method-over-resolveshorthand) + - [render methods](#render-methods) + - [Types](#types) + - [Introduction of the `SlotComponent` type.](#introduction-of-the-slotcomponent-type) + - [ComponentState](#componentstate) + - [Styles hooks](#styles-hooks) + - [Option 1 mutate external properties](#option-1-mutate-external-properties) + - [Option 2 mutate overrides](#option-2-mutate-overrides) + - [Option 3 stop mutating](#option-3-stop-mutating) + - [Passing overrides from state to render](#passing-overrides-from-state-to-render) + - [Custom Pragma implementation](#custom-pragma-implementation) + - [Pros and Cons](#pros-and-cons) + - [Pros](#pros) + - [Cons](#cons) + - [Option B: Refactor getSlots + helper method](#option-b-refactor-getslots--helper-method) + - [Pros and Cons](#pros-and-cons-1) + - [Pros](#pros-1) + - [Cons](#cons-1) + - [Option C: Custom JSX Pragma without internal changes](#option-c-custom-jsx-pragma-without-internal-changes) + - [Pros and Cons](#pros-and-cons-2) + - [Pros](#pros-2) + - [Cons](#cons-2) + - [Option D: Option A (partially) + Option C](#option-d-option-a-partially--option-c) + - [Pros](#pros-3) + - [Cons](#cons-3) + + + ## TL;DR Slot children render function is a complex API that is not properly supported. To support this the best alternative so far, From b9997a1a8ce760ea208177123547e29b152b86a4 Mon Sep 17 00:00:00 2001 From: Bernardo Sunderhus Date: Thu, 23 Mar 2023 13:37:47 +0000 Subject: [PATCH 17/19] chore: improves documentation for option A --- .../slot-children-render-function.md | 105 ++++++++++-------- 1 file changed, 57 insertions(+), 48 deletions(-) diff --git a/rfcs/react-components/convergence/slot-children-render-function.md b/rfcs/react-components/convergence/slot-children-render-function.md index fb0c1caea6870..6b2473ec07b5e 100644 --- a/rfcs/react-components/convergence/slot-children-render-function.md +++ b/rfcs/react-components/convergence/slot-children-render-function.md @@ -7,47 +7,45 @@ _@bsunderhus @ling1726 @layershifter_ -**Table of Contents** - -- [RFC: Slot children render function](#rfc-slot-children-render-function) - - [TL;DR](#tldr) - - [Background](#background) - - [External properties](#external-properties) - - [Internal properties](#internal-properties) - - [Overrides](#overrides) - - [What happens behind the scenes?](#what-happens-behind-the-scenes) - - [Current issues](#current-issues) - - [Problem statement](#problem-statement) - - [Problem 1](#problem-1) - - [Problem 2](#problem-2) - - [Sum up](#sum-up) - - [Detailed Design or Proposal](#detailed-design-or-proposal) - - [Option A: Custom JSX Pragma](#option-a-custom-jsx-pragma) +- [TL;DR](#tldr) +- [Background](#background) + - [External properties](#external-properties) + - [Internal properties](#internal-properties) + - [Overrides](#overrides) + - [What happens behind the scenes?](#what-happens-behind-the-scenes) +- [Current issues](#current-issues) +- [Problem statement](#problem-statement) + - [Problem 1](#problem-1) + - [Problem 2](#problem-2) + - [Sum up](#sum-up) +- [Detailed Design or Proposal](#detailed-design-or-proposal) + - [Option A: Custom JSX Pragma](#option-a-custom-jsx-pragma) + - [Required changes](#required-changes) - [slot method over resolveShorthand](#slot-method-over-resolveshorthand) - - [render methods](#render-methods) - - [Types](#types) + - [simplify render methods](#simplify-render-methods) + - [SlotComponent type & ComponentState change](#slotcomponent-type--componentstate-change) - [Introduction of the `SlotComponent` type.](#introduction-of-the-slotcomponent-type) - [ComponentState](#componentstate) - - [Styles hooks](#styles-hooks) + - [Styles hooks `.props` access](#styles-hooks-props-access) - [Option 1 mutate external properties](#option-1-mutate-external-properties) - [Option 2 mutate overrides](#option-2-mutate-overrides) - [Option 3 stop mutating](#option-3-stop-mutating) - - [Passing overrides from state to render](#passing-overrides-from-state-to-render) - - [Custom Pragma implementation](#custom-pragma-implementation) - - [Pros and Cons](#pros-and-cons) - - [Pros](#pros) - - [Cons](#cons) - - [Option B: Refactor getSlots + helper method](#option-b-refactor-getslots--helper-method) - - [Pros and Cons](#pros-and-cons-1) - - [Pros](#pros-1) - - [Cons](#cons-1) - - [Option C: Custom JSX Pragma without internal changes](#option-c-custom-jsx-pragma-without-internal-changes) - - [Pros and Cons](#pros-and-cons-2) - - [Pros](#pros-2) - - [Cons](#cons-2) - - [Option D: Option A (partially) + Option C](#option-d-option-a-partially--option-c) - - [Pros](#pros-3) - - [Cons](#cons-3) + - [Passing overrides from state to render when needed](#passing-overrides-from-state-to-render-when-needed) + - [Custom Pragma implementation](#custom-pragma-implementation) + - [Pros and Cons](#pros-and-cons) + - [Pros](#pros) + - [Cons](#cons) + - [Option B: Refactor getSlots + helper method](#option-b-refactor-getslots--helper-method) + - [Pros and Cons](#pros-and-cons-1) + - [Pros](#pros-1) + - [Cons](#cons-1) + - [Option C: Custom JSX Pragma without internal changes](#option-c-custom-jsx-pragma-without-internal-changes) + - [Pros and Cons](#pros-and-cons-2) + - [Pros](#pros-2) + - [Cons](#cons-2) + - [Option D: Option A (partially) + Option C](#option-d-option-a-partially--option-c) + - [Pros](#pros-3) + - [Cons](#cons-3) @@ -293,11 +291,22 @@ The last provided properties are the Overrides, which are provided on rendering Since we only have access to the Overrides on the declaration of the jsx element itself, the only way to postpone merging of the properties after we have the Overrides is by changing the intrinsic mechanism of how a jsx element is consumed. +This can be done by providing a custom [JSX pragma](https://www.gatsbyjs.com/blog/2019-08-02-what-is-jsx-pragma/). -Instead of having a `resolveShorthand` that will return a merge between External properties and Internal properties, +Instead of having a `resolveShorthand` that returns a merge between External properties and Internal properties, we can provide a custom exotic component, similar to what happens when `React.memo` or `React.forwardRef` does. -#### slot method over resolveShorthand +#### Required changes + +We can split this proposal by the required changes that need to be done. They can be listed as: + +1. [slot method over resolveShorthand](#slot-method-over-resolveshorthand) +2. [simplify render methods](#simplify-render-methods) +3. [SlotComponent type & ComponentState change](#slotcomponent-type--componentstate-change) +4. [Styles hooks `.props` access](#styles-hooks-props-access) +5. [Passing overrides from state to render when needed](#passing-overrides-from-state-to-render-when-needed) + +##### `slot` over `resolveShorthand` The `slot` method will return a slot component, similar to a component declaration when using `React.memo` or `React.forwardRef`, and in that slot component declaration, both External properties and Internal properties will coexist without merging, @@ -333,10 +342,10 @@ const state = { }; ``` -#### render methods +##### simplify render methods -Since there'll be a custom pragma, there's no need for `getSlots` anymore, the components provided by the `slot` method will -be renderable, meaning we can simply use them in the render! +Since there'll be a custom pragma which will understand what is provided by `slot`, there's no need for `getSlots` anymore! +The components provided by the `slot` method will be renderable, meaning we can simply use them in the render! ```tsx export const renderAccordionHeader_unstable = (state: AccordionHeaderState) => ( @@ -351,11 +360,11 @@ export const renderAccordionHeader_unstable = (state: AccordionHeaderState) => ( ); ``` -#### Types +##### SlotComponent type & ComponentState change Some type changes will be required: -##### Introduction of the `SlotComponent` type. +###### Introduction of the `SlotComponent` type. Slot components are exotic components (like `React.forwardRef` and `React.memo` components), that will have intrinsically associated with them External properties, Internal properties and the base component type. @@ -372,7 +381,7 @@ type SlotComponent = React.Ex }; ``` -##### ComponentState +###### ComponentState `ComponentState` should map provided slots to `SlotComponent`s instead of mapping to resolved shorthands! And `components` property can be dropped. @@ -402,14 +411,14 @@ type ComponentState = { }; ``` -#### Styles hooks +##### Styles hooks `.props` access A minor modification in style hooks are required. at the moment we mutate the merged properties provided by `resolveShorthand` to include our custom `className` property. In this case we have some alternatives: -##### Option 1 mutate external properties +###### Option 1 mutate external properties Instead of mutating the merged properties, simply mutate the External properties from the slot component: @@ -441,7 +450,7 @@ export const useAccordionHeaderStyles_unstable = (state: AccordionHeaderState) = }; ``` -##### Option 2 mutate overrides +###### Option 2 mutate overrides By introducing an `overrides` property on the `ComponentState` we can create a layer of override that can be mutated: @@ -488,7 +497,7 @@ export const renderAccordionHeader_unstable = (state: AccordionHeaderState) => ( ); ``` -##### Option 3 stop mutating +###### Option 3 stop mutating Let's just stop mutating, and since we require a layer of custom styling through classNames let's just properly use an argument on the render method exclusively for this: @@ -557,7 +566,7 @@ export const AccordionHeader: ForwardRefComponent = React. ); ``` -#### Passing overrides from state to render +##### Passing overrides from state to render when needed Since our logic lies on the state hooks, the override methods will be implemented on those state hooks, although it should be declared on the render method explicitly to properly work as an override. From 4fc90330a4e03a5b6038955c2f24f8dfc506e6b4 Mon Sep 17 00:00:00 2001 From: Bernardo Sunderhus Date: Thu, 23 Mar 2023 13:44:05 +0000 Subject: [PATCH 18/19] improves simplify render methods section --- .../slot-children-render-function.md | 28 ++++++++++++++++--- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/rfcs/react-components/convergence/slot-children-render-function.md b/rfcs/react-components/convergence/slot-children-render-function.md index 6b2473ec07b5e..7e069cb0c0065 100644 --- a/rfcs/react-components/convergence/slot-children-render-function.md +++ b/rfcs/react-components/convergence/slot-children-render-function.md @@ -21,8 +21,8 @@ _@bsunderhus @ling1726 @layershifter_ - [Detailed Design or Proposal](#detailed-design-or-proposal) - [Option A: Custom JSX Pragma](#option-a-custom-jsx-pragma) - [Required changes](#required-changes) - - [slot method over resolveShorthand](#slot-method-over-resolveshorthand) - - [simplify render methods](#simplify-render-methods) + - [`slot` over `resolveShorthand`](#slot-over-resolveshorthand) + - [simplify render methods (get rid of `getSlots`)](#simplify-render-methods-get-rid-of-getslots) - [SlotComponent type & ComponentState change](#slotcomponent-type--componentstate-change) - [Introduction of the `SlotComponent` type.](#introduction-of-the-slotcomponent-type) - [ComponentState](#componentstate) @@ -301,7 +301,7 @@ we can provide a custom exotic component, similar to what happens when `React.me We can split this proposal by the required changes that need to be done. They can be listed as: 1. [slot method over resolveShorthand](#slot-method-over-resolveshorthand) -2. [simplify render methods](#simplify-render-methods) +2. [simplify render methods (get rid of `getSlots`)](#simplify-render-methods-get-rid-of-getslots) 3. [SlotComponent type & ComponentState change](#slotcomponent-type--componentstate-change) 4. [Styles hooks `.props` access](#styles-hooks-props-access) 5. [Passing overrides from state to render when needed](#passing-overrides-from-state-to-render-when-needed) @@ -342,11 +342,31 @@ const state = { }; ``` -##### simplify render methods +##### simplify render methods (get rid of `getSlots`) Since there'll be a custom pragma which will understand what is provided by `slot`, there's no need for `getSlots` anymore! The components provided by the `slot` method will be renderable, meaning we can simply use them in the render! +Before: + +```tsx +export const renderAccordionHeader_unstable = (state: AccordionHeaderState) => { + const { slots, slotProps } = getSlots(state); + return ( + + + {state.expandIconPosition === 'start' && slots.expandIcon && } + {slots.icon && } + {slotProps.root.props.children} + {state.expandIconPosition === 'end' && slots.expandIcon && } + + + ); +}; +``` + +After: + ```tsx export const renderAccordionHeader_unstable = (state: AccordionHeaderState) => ( From 7fc2f4788aa5f91bb4b2291955879f8c4bb3e031 Mon Sep 17 00:00:00 2001 From: Bernardo Sunderhus Date: Thu, 23 Mar 2023 20:02:34 +0000 Subject: [PATCH 19/19] chore: update problem 1 text --- .../convergence/slot-children-render-function.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rfcs/react-components/convergence/slot-children-render-function.md b/rfcs/react-components/convergence/slot-children-render-function.md index 7e069cb0c0065..eccd708e46fb3 100644 --- a/rfcs/react-components/convergence/slot-children-render-function.md +++ b/rfcs/react-components/convergence/slot-children-render-function.md @@ -218,8 +218,8 @@ The `slot` argument will represent the merging between `state.component.button` as the name suggests is all the properties provided by the `resolveShorthand` invocation without including `as` and also `children` (since `children` by itself is the render function). -Since we lost the Internal properties on the merging between it and the External ones -There's no way to provide such the default declared `children` to the render function! +Since we lost the Internal properties (`defaultProps`) on the merging with the External properties, +there's no way to provide `defaultProps.children` to the render function! ```js function useComponent(props) {