feat(react-jsx-runtime): implements next steps (option D) #27753
Conversation
📊 Bundle size report
Unchanged fixtures
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 65f0125dd6a85c3c772d63ad3350561af77addbc (build) |
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 578 | 599 | 5000 | Possible regression |
| Button | mount | 294 | 302 | 5000 | Possible regression |
| InfoButton | mount | 17 | 17 | 5000 | Possible regression |
| SpinButton | mount | 1279 | 1334 | 5000 | Possible regression |
All results
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 578 | 599 | 5000 | Possible regression |
| Button | mount | 294 | 302 | 5000 | Possible regression |
| Field | mount | 1009 | 1051 | 5000 | |
| FluentProvider | mount | 659 | 669 | 5000 | |
| FluentProviderWithTheme | mount | 78 | 85 | 10 | |
| FluentProviderWithTheme | virtual-rerender | 72 | 72 | 10 | |
| FluentProviderWithTheme | virtual-rerender-with-unmount | 76 | 76 | 10 | |
| InfoButton | mount | 17 | 17 | 5000 | Possible regression |
| MakeStyles | mount | 904 | 875 | 50000 | |
| Persona | mount | 1601 | 1654 | 5000 | |
| SpinButton | mount | 1279 | 1334 | 5000 | Possible regression |
|
It would be good to summarize changes required from consumers/developers to upgrade to a new approach |
| ref, | ||
| ...props, | ||
| }), | ||
| { required: true, componentType: 'div' }, |
There was a problem hiding this comment.
| { required: true, componentType: 'div' }, | |
| { componentType: 'div' }, |
Is there sense to use required? It will be always defined anyway, correct?
There was a problem hiding this comment.
undefined is still a possible value if required is not provided and NonNullable is not assigned in the type. This is important to be clear also, required by itself doesn't do much, you gotta put it side by side together with the NonNullable type on the slot definition.
|
|
||
| export type DialogContentSlots = { | ||
| root: Slot<'div'>; | ||
| root: NonNullable<Slot<'div'>>; |
There was a problem hiding this comment.
Will it explode if we will keep previous definition? I.e. root: Slot<'div'>;
There was a problem hiding this comment.
Yes. the previous definition is simply wrong. root slot should be non nullable and declared as required. since from this PR and forward we're finally treating root as proper slot if we don't properly declare it as non nullable and required we'll start to see it as optional in the render method.
|
|
||
| import { createElement } from '@fluentui/react-jsx-runtime'; | ||
| import { createElementNext } from '@fluentui/react-jsx-runtime'; |
There was a problem hiding this comment.
It's draft, so it's okay to use createElementNext, but for a final change let's consider better naming
| onKeyDown: handleKeyDown, | ||
| ref: useMergedRefs(ref, dialogRef), | ||
| }), | ||
| backdrop: backdropSlot, |
There was a problem hiding this comment.
Is there a reason why you created backdropSlot variable?
There was a problem hiding this comment.
I believe it was for the case of not having the overrides option. Because in that case you'd need a reference to the slot component definition to then modify it's properties.
There was a problem hiding this comment.
But honestly, I see no harm in having overrides property there, it makes things easier
There was a problem hiding this comment.
But honestly, I see no harm in having overrides property there, it makes things easier
How overrides makes it easier? :)
const slot = slot(props.slot, {
overrides: {
onClick: useEventCallback(() => {
// Hm.. What should I call?
if (typeof props.slot === 'object' && props.slot !== null) {
props.slot.onClick()
}
})
}
})Do we really want to do that?
There was a problem hiding this comment.
typeof props.slot === 'object' && props.slot !== null this is the only problem I see, and it's unnecessary in this scenario, we have isResolvedShorthand to avoid that.
There was a problem hiding this comment.
Correct, but following will be simpler and does not require checks (and we are doing this anyway):
const foo = slot()
foo.onClick = mergeCallbacks(foo.onClick, () => {})
| id: useDialogContext_unstable(ctx => ctx.dialogTitleId), | ||
| ...props, | ||
| }), | ||
| { componentType: defaultComponentType, required: true }, |
There was a problem hiding this comment.
| { componentType: defaultComponentType, required: true }, | |
| { componentType: as, required: true }, |
Why not as?
There was a problem hiding this comment.
It actually should be as in that case! otherwise there's no way to alter root base type, since getNativeElementProps will do us the favor of removing the as property 🥲
| getNativeElementProps(as, { | ||
| ref, | ||
| id: useDialogContext_unstable(ctx => ctx.dialogTitleId), | ||
| ...props, | ||
| }), | ||
| { componentType: defaultComponentType, required: true }, |
There was a problem hiding this comment.
getNativeElementProps(as /* element type */, {}),
{ componentType: defaultComponentType /* element type */, required: true },This smells a bit, looks we can consider to create a special function for root slot or embed props filtering to slot()
There was a problem hiding this comment.
Or we could just stop prop filtering?! 👀 as we have concise types to avoid unnecessary properties?!
There was a problem hiding this comment.
It makes absolutely no sense to embed props filtering to slot, it has proven to be unnecessary for every single slot and that is precisely why we don't have it in all implementations, I do believe we can move forward to a way that we stop treating root as something different than just another slot, let's just stop prop filtering root.
There was a problem hiding this comment.
What are the impediments stopping us from opting out of not filtering props for the root slot?
There was a problem hiding this comment.
What are the impediments stopping us from opting out of not filtering props for the root slot?
We will need to filter out component's props from props manually, which might not be so bad.
| ...props, | ||
| }), | ||
| action: resolveShorthand(action, { | ||
| root: slot<DialogTitleProps>( |
There was a problem hiding this comment.
Why do you need to specify DialogTitleProps explicitly?
There was a problem hiding this comment.
You don't actually. if you remove it'll work. I guess it's there for some implementation reminiscent
|
|
||
| const slotComponent = { | ||
| ...defaultProps, | ||
| $$typeof: SLOT_COMPONENT_TYPEOF_SYMBOL, |
There was a problem hiding this comment.
What does $$typeof? I removed it together with SLOT_COMPONENT_TYPEOF_SYMBOL and everything still works 🤔
There was a problem hiding this comment.
It is a type that is defined by React.ExoticComponent, every single exotic component has that property to help react to identify what component type is that. We can remove it for sure, I just kept it there to maintain implementation similar to what react does.
There was a problem hiding this comment.
Let's remove it as it does nothing for this implementation.
| @@ -0,0 +1,34 @@ | |||
| import * as React from 'react'; | |||
| import { isSlot, UnknownSlotProps, SLOT_COMPONENT_METADATA_SYMBOL } from '@fluentui/react-utilities'; | |||
| import { SlotComponent } from '@fluentui/react-utilities/src/compose/types'; | |||
There was a problem hiding this comment.
| import { SlotComponent } from '@fluentui/react-utilities/src/compose/types'; | |
| import { SlotComponent } from '@fluentui/react-utilities'; |
| } | ||
| } | ||
|
|
||
| return Object.assign(slotComponent, overrides); |
There was a problem hiding this comment.
| return Object.assign(slotComponent, overrides); | |
| return slotComponent; |
Let's remove overrides as a concept from this implementation. It had sense in Northstar as factories returned there a React element, but here it returns props. With overrides there will be two ways of doing things:
const a = slot(props.a)
slot.foo = 'foo'
// vs
const a = slot(props.a, { overrides: { foo: 'foo' })What is a correct way? And would it should be done via overrides? 🐱
There was a problem hiding this comment.
Well, yeah. we can remove it for sure. #27753 (comment), this becomes necessary though.
And in the backdrop it'll require some conditional as it's an optional slot:
const backdropSlot = slot(backdrop, {
componentType: 'div',
required: open && modalType !== 'non-modal',
defaultProps: { 'aria-hidden': 'true' },
});
if (backdropSlot) {
backdropSlot.onClick = handledBackdropClick;
}
return {
backdrop: backdropSlot,
|
I am in favor of Option D (this PR)
Anyway, before we will do any changes we need to ensure that a new release with these changes will not break consumers |
| ref: useMergedRefs(ref, dialogRef), | ||
| }), | ||
| backdrop: backdropSlot, | ||
| root: slot<DialogSurfaceProps>( |
There was a problem hiding this comment.
As componentType is a required param, is there a reason to keep it options? Did you consider following?
// Option 1
// slot(COMPONENT_TYPE, PROPS, OPTIONS)
slot('div', props.slot, { required: true })
// Option 2
// slot(OPTIONS)
slot({ elementType: 'div', props: props.slot, required: true })There was a problem hiding this comment.
Option 1 I'm not a big fan, 3 arguments on a function brings a lot of options.
I'd be ok with Option 2, looks even simpler for me, one less argument
There was a problem hiding this comment.
here's one thing though.... on Option 2 you create the property props, that indicates we're going to be passing the props for the slot there, but we don't know if we have props, we only have props.slot which is a shorthand (which might be props of the slot, or might be only children, or null, or undefined). soooo, should we call it shorthand?
There was a problem hiding this comment.
should we call it shorthand?
Or may be just value...
| if (shorthand === null || (shorthand === undefined && !required)) { | ||
| return undefined; | ||
| } | ||
| const metadata: SlotComponentMetadata<Props> = { componentType }; |
There was a problem hiding this comment.
Out of curiosity, why componentType instead of elementType?
There was a problem hiding this comment.
Just because 🤷🏼♂️. I guess it's because we have components property on the state and that would be something similar?! but yeah, elementType is more concise
bsunderhus
force-pushed
the
react-jsx-runtime/feat--implements-next-steps-(option-D)
branch
2 times, most recently
from
7c79e9f
to
cfd0e7a
Compare
bsunderhus
force-pushed
the
react-jsx-runtime/feat--implements-next-steps-(option-D)
branch
from
ce22db7
to
91fa887
Compare
| 'aria-hidden': 'true', | ||
| onClick: handledBackdropClick, | ||
| backdrop: backdropSlot, | ||
| root: rootSlot<DialogSurfaceSlots>({ |
There was a problem hiding this comment.
Looking on it now, I am thinking may be we can be more explicit. Here is an example:
const state = {
root: slot.root(),
backdrop: slot.optional(),
foo: slot.required()
}The downside that I see: it will be harder to have conditions for required/optinal as you have in this component.
@bsunderhus WDYT?
There was a problem hiding this comment.
I like making this more explicit and not having a default of "required: false". This part of the slots API always seems to confuse devs working on components.
There are actually three different cases:
- optional: only rendered if a user value is provided for the slot (unless they set the slot to
null). - nullable: rendered by default, but can be set to
nullby the user to prevent rendering.- In the current API, this is the case if you pass
required: trueunless you also addNonNullableto the slot type.
- In the current API, this is the case if you pass
- required: always rendered; can't be set to null.
Splitting out the three cases into different slot functions would make it harder to mess up, especially mixing up nullable vs. required (since we could enforce that the slot is NonNullable if passed to the required slot function).
If we had that, I don't think we'd need to special-case the root slot; it'd just be a "required" slot.
There was a problem hiding this comment.
It would be really great to come up with better names for the three cases as well... As always, naming is the hardest part of software engineering.
| @@ -244,6 +281,10 @@ export function useScrollbarWidth(options: UseScrollbarWidthOptions): number | u | |||
| // @internal | |||
| export function useTimeout(): readonly [(fn: () => void, delay: number) => void, () => void]; | |||
|
|
|||
| // Warnings were encountered during analysis: | |||
| // | |||
| // /workspaces/fluentui/dist/out-tsc/types/packages/react-components/react-utilities/src/compose/types.d.ts:169:5 - (ae-incompatible-release-tags) The symbol "[SLOT_COMPONENT_METADATA_SYMBOL]" is marked as @public, but its signature references "SLOT_COMPONENT_METADATA_SYMBOL" which is marked as @internal | |||
bsunderhus
force-pushed
the
react-jsx-runtime/feat--implements-next-steps-(option-D)
branch
from
91fa887
to
371489a
Compare
| export function rootSlot<Slots extends SlotPropsRecord, Primary extends keyof Slots = 'root'>( | ||
| options: SlotOptions<ExtractSlotProps<Slots[Primary]>> & { | ||
| props: PropsWithoutRef<ExtractSlotProps<Slots[Primary]>>; | ||
| ref: 'ref' extends keyof ExtractSlotProps<Slots[Primary]> | ||
| ? ExtractSlotProps<Slots[Primary]>['ref'] | ||
| : React.Ref<HTMLElement>; | ||
| }, | ||
| ): SlotComponent<ExtractSlotProps<Slots[Primary]>> { | ||
| const { defaultProps, elementType } = options; | ||
| const props: ExtractSlotProps<Slots[Primary]> = getNativeElementProps(elementType as string, { | ||
| ref: options.ref, | ||
| ...options.props, | ||
| }); | ||
| return slot({ shorthand: props, elementType, defaultProps, required: true }); | ||
| } |
There was a problem hiding this comment.
This function is called rootSlot but it appears to be getting the primary slot's props (not root), and doesn't seem to do proper prop splitting when the primary slot is not root.
Also, building in getNativeElementProps here won't work in general: Some components need to pass value to excludedPropNames. Or others use getPartitionedNativeProps (if the primary slot is not root). And some components don't have a native element as the root slot, so do manual prop splitting. Can we have callers do the prop splitting before calling rootSlot?
There was a problem hiding this comment.
If we go with the suggestion from this comment: https://github.com/microsoft/fluentui/pull/27753/files#r1194833022, then we don't need a rootSlot() function. It should work to use slot.required().
| 'aria-hidden': 'true', | ||
| onClick: handledBackdropClick, | ||
| backdrop: backdropSlot, | ||
| root: rootSlot<DialogSurfaceSlots>({ |
There was a problem hiding this comment.
It would be really great to come up with better names for the three cases as well... As always, naming is the hardest part of software engineering.
packages/react-components/react-jsx-runtime/src/createElementNext.ts
Outdated
Show resolved
Hide resolved
| export function slot<Props extends UnknownSlotProps = UnknownSlotProps>( | ||
| options: { shorthand: Props | SlotShorthandValue | undefined | null; required?: boolean } & SlotOptions<Props>, | ||
| ): SlotComponent<Props> | undefined { |
There was a problem hiding this comment.
It looks like this slot function is saving the render function in a different way than resolveShorthand (on a metadata object vs. on SLOT_RENDER_FUNCTION_SYMBOL).
That's fine in general, but it would be nice to keep interoperability between calls to resolveShorthand and slot. The main reason is that resolveShorthand is the only way for a component to modify slot props of a different component. E.g. if a Foo component has a slot of type Bar, and wants to modify the props sent to its bar slot, it would need to call resolveShorthand on the bar slot first. Here's an example where a component needs to modify the slot of another component: #27834
Here's a contrived stress test case that should work. Multiple repeated calls to resolveShorthand followed by a call to slot:
const component = <TestComponent mySlot={{ children: () => { /* render function */ }} />;
// Inside TestComponent:
// The first call moves the render function to `SLOT_RENDER_FUNCTION_SYMBOL`:
const mySlotResolved1 = resolveShorthand(props.mySlot);
// The second call should maintain the render function.
const mySlotResolved2 = resolveShorthand(mySlotResolved1);
// The call to slot should also maintain the render function.
const mySlot = slot({ shorthand: mySlotResolved2 });
// When rendering the component with mySlot, it should use the render function.I think in order for that to work, slot would need to check for the existence of SLOT_RENDER_FUNCTION_SYMBOL and make sure it ends up in the right place on the metadata.
| }; | ||
|
|
||
| export function slot<Props extends UnknownSlotProps = UnknownSlotProps>( | ||
| options: { shorthand: Props | SlotShorthandValue | undefined; required: true } & SlotOptions<Props>, |
There was a problem hiding this comment.
The "shorthand" argument is not necessarily a shorthand value. Not sure what the best name would be. Perhaps value, which is the name that resolveShorthand uses.
Also, super-nit, the object should be called "params" or something (and the type "SlotParams"), rather than "options"/"SlotOptions" since they are not optional 🙂.
| options: { shorthand: Props | SlotShorthandValue | undefined; required: true } & SlotOptions<Props>, | |
| params: { value: Props | SlotShorthandValue | undefined; required: true } & SlotParams<Props>, |
| function createElementFromSlotComponent<Props extends UnknownSlotProps>( | ||
| type: SlotComponent<Props>, | ||
| overrideChildren: React.ReactNode[], | ||
| ): React.ReactElement<Props> | null { |
There was a problem hiding this comment.
It looks like createElementFromSlotComponent is ignoring any props set in JSX. Setting slot props in JSX is definitely not recommended, but since I don't think we can prevent it via TypeScript, then we should probably handle it here. Otherwise it will silently fail to work, and could be very confusing to a dev.
I.e. this should work, and result in the foo prop being overridden to be "bar":
<state.mySlot foo="bar" />There was a problem hiding this comment.
Setting slot props in JSX is definitely not recommended, but since I don't think we can prevent it via TypeScript, then we should probably handle it here. Otherwise it will silently fail to work, and could be very confusing to a dev.
We can prevent it via Typescript! 💪🏻 The SlotComponent extending React.ExoticComponent<React.PropsWithChildren<{}>> makes sure that we're enforcing JSX props to be React.PropsWithChildren<{}> in other words, we'll be enforcing through Typescript that slots only support children as a valid property on override time, ensuring the only possible way to add properties to a slot is through SlotComponent creation (by slot methods)
bsunderhus
force-pushed
the
react-jsx-runtime/feat--implements-next-steps-(option-D)
branch
from
45f8cfd
to
90c3459
Compare
packages/react-components/react-accordion/src/components/Accordion/renderAccordion.tsx
Show resolved
Hide resolved
bsunderhus
force-pushed
the
react-jsx-runtime/feat--implements-next-steps-(option-D)
branch
from
90c3459
to
5a5688b
Compare
packages/react-components/react-accordion/src/components/Accordion/renderAccordion.tsx
Show resolved
Hide resolved
bsunderhus
force-pushed
the
react-jsx-runtime/feat--implements-next-steps-(option-D)
branch
from
5a5688b
to
dc6ca6c
Compare
packages/react-components/react-accordion/src/components/Accordion/renderAccordion.tsx
Show resolved
Hide resolved
bsunderhus
force-pushed
the
react-jsx-runtime/feat--implements-next-steps-(option-D)
branch
2 times, most recently
from
aea42bb
to
f01e04f
Compare
🕵 fluentuiv9 No visual regressions between this PR and main |
bsunderhus
force-pushed
the
react-jsx-runtime/feat--implements-next-steps-(option-D)
branch
14 times, most recently
from
1f50ecd
to
401adf5
Compare
bsunderhus
force-pushed
the
react-jsx-runtime/feat--implements-next-steps-(option-D)
branch
from
401adf5
to
b1398fa
Compare
Previous Behavior
New Behavior
Related Issue(s)