Simplify prop merging with slots by bsunderhus · Pull Request #18721 · microsoft/fluentui

bsunderhus · 2021-06-25T14:31:22Z

Pull request checklist

Include a change request file using $ yarn change

Description of changes

Implements changes proposed by the RFC #18642

To sum up the RFC:

This RFC proposes a set of changes in three major steps:

Fixing the use of slots in mergeProps and usage of the as prop, so that JSX elements in slots can be configured using shorthand as prop.
Stricter typing for shorthand slots that render native HTML tags with the goal of improving type safety internally and for consumers.
A logical conclusion of the two above steps which results completely removing mergeProps in favour of object spreading over deep merge.

PREVIEW 📃

Problems so far in this implementation

The amount of inferences happening due to JSX.IntrinsicElements is harmful. Need to find a way to force developer to pass down the way Typings instead of letting Typescript compiler infer things by itself
It's simply impossible to properly infer typings for root. root is completely alien to the new system of typings, we should consider using an approach of declaring it the same way all the other slots are declared in state.
Some components, like CompoundButton and AccordionHeader break the internal logic of getSlots to decide if a slot should or should not be rendered (this is not a problem of this implementation! this is legacy).

To addres incomplete problems new RFC's might be proposed since they'll create breaking changes

size-auditor · 2021-06-25T14:42:29Z

Asset size changes

Project	Bundle	Baseline Size	New Size	Difference
office-ui-fabric-react	fluentui-react-components-Link	14.709 kB	14.708 kB	-1 bytes
office-ui-fabric-react	fluentui-react-components-ToggleButton	35.424 kB	35.42 kB	-4 bytes
office-ui-fabric-react	fluentui-react-components-Divider	15.893 kB	15.889 kB	-4 bytes
office-ui-fabric-react	fluentui-react-components-CompoundButton	29.95 kB	29.946 kB	-4 bytes
office-ui-fabric-react	fluentui-react-components-MenuButton	26.784 kB	26.78 kB	-4 bytes

Over Tolerance (1024 B) Over Baseline Below Baseline New Removed 1 kB = 1000 B

Baseline commit: 27cc926310490c4291a44b2e399547a95aa3dc84 (build)

fabricteam · 2021-06-25T15:12:57Z

Perf Analysis (`@fluentui/react`)

No significant results to display.

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations
Avatar	mount	927	917	5000
BaseButton	mount	1039	1076	5000
Breadcrumb	mount	2896	2922	1000
ButtonNext	mount	610	581	5000
Checkbox	mount	1707	1711	5000
CheckboxBase	mount	1516	1468	5000
ChoiceGroup	mount	5360	5335	5000
ComboBox	mount	1069	1112	1000
CommandBar	mount	11407	11355	1000
ContextualMenu	mount	7015	6986	1000
DefaultButton	mount	1303	1308	5000
DetailsRow	mount	4184	4153	5000
DetailsRowFast	mount	4286	4319	5000
DetailsRowNoStyles	mount	3904	4018	5000
Dialog	mount	2386	2293	1000
DocumentCardTitle	mount	172	158	1000
Dropdown	mount	3673	3714	5000
FluentProviderNext	mount	7466	7502	5000
FocusTrapZone	mount	1971	1974	5000
FocusZone	mount	1921	2001	5000
IconButton	mount	1983	2034	5000
Label	mount	378	375	5000
Layer	mount	2077	2099	5000
Link	mount	532	529	5000
MakeStyles	mount	2013	2026	50000
MenuButton	mount	1736	1668	5000
MessageBar	mount	2198	2232	5000
Nav	mount	3680	3781	1000
OverflowSet	mount	1159	1174	5000
Panel	mount	2271	2267	1000
Persona	mount	910	913	1000
Pivot	mount	1582	1646	1000
PrimaryButton	mount	1486	1438	5000
Rating	mount	8972	8933	5000
SearchBox	mount	1497	1533	5000
Shimmer	mount	2879	2901	5000
Slider	mount	2193	2254	5000
SpinButton	mount	5644	5633	5000
Spinner	mount	458	473	5000
SplitButton	mount	3548	3633	5000
Stack	mount	563	566	5000
StackWithIntrinsicChildren	mount	1819	1778	5000
StackWithTextChildren	mount	5221	5282	5000
SwatchColorPicker	mount	11555	11546	5000
Tabs	mount	1600	1574	1000
TagPicker	mount	2813	2776	5000
TeachingBubble	mount	13157	13086	5000
Text	mount	494	471	5000
TextField	mount	1591	1545	5000
ThemeProvider	mount	1327	1326	5000
ThemeProvider	virtual-rerender	672	672	5000
Toggle	mount	912	927	5000
buttonNative	mount	133	123	5000

Perf Analysis (`@fluentui/react-northstar`)

⚠️ 1 potential perf regressions detected

Potential regressions comparing to master

Scenario	Current PR Ticks	Baseline Ticks	Ratio	Regression Analysis
PortalMinimalPerf.default	199	180	1.11:1	analysis

Perf tests with no regressions

Scenario	Current PR Ticks	Baseline Ticks	Ratio
AttachmentMinimalPerf.default	196	177	1.11:1
FormMinimalPerf.default	525	476	1.1:1
LayoutMinimalPerf.default	451	412	1.09:1
AnimationMinimalPerf.default	503	464	1.08:1
DropdownManyItemsPerf.default	837	777	1.08:1
RefMinimalPerf.default	270	250	1.08:1
ImageMinimalPerf.default	474	443	1.07:1
SkeletonMinimalPerf.default	446	415	1.07:1
TreeWith60ListItems.default	207	194	1.07:1
ButtonSlotsPerf.default	655	616	1.06:1
LabelMinimalPerf.default	466	441	1.06:1
BoxMinimalPerf.default	424	405	1.05:1
RadioGroupMinimalPerf.default	549	522	1.05:1
ChatMinimalPerf.default	792	759	1.04:1
ChatWithPopoverPerf.default	426	411	1.04:1
SliderMinimalPerf.default	1797	1724	1.04:1
StatusMinimalPerf.default	824	791	1.04:1
IconMinimalPerf.default	739	713	1.04:1
CarouselMinimalPerf.default	550	536	1.03:1
HeaderSlotsPerf.default	919	890	1.03:1
ItemLayoutMinimalPerf.default	1456	1408	1.03:1
ListMinimalPerf.default	607	591	1.03:1
MenuButtonMinimalPerf.default	1863	1811	1.03:1
SegmentMinimalPerf.default	418	407	1.03:1
ChatDuplicateMessagesPerf.default	344	336	1.02:1
FlexMinimalPerf.default	330	324	1.02:1
ListNestedPerf.default	662	652	1.02:1
PopupMinimalPerf.default	636	626	1.02:1
SplitButtonMinimalPerf.default	4309	4244	1.02:1
TextAreaMinimalPerf.default	605	594	1.02:1
AlertMinimalPerf.default	329	326	1.01:1
AvatarMinimalPerf.default	234	232	1.01:1
ButtonMinimalPerf.default	206	203	1.01:1
CardMinimalPerf.default	655	651	1.01:1
DialogMinimalPerf.default	881	875	1.01:1
DividerMinimalPerf.default	421	416	1.01:1
DropdownMinimalPerf.default	3412	3378	1.01:1
InputMinimalPerf.default	1430	1412	1.01:1
ListWith60ListItems.default	745	740	1.01:1
MenuMinimalPerf.default	995	985	1.01:1
RosterPerf.default	1332	1313	1.01:1
ProviderMinimalPerf.default	1137	1131	1.01:1
TableManyItemsPerf.default	2263	2248	1.01:1
TextMinimalPerf.default	420	415	1.01:1
CustomToolbarPrototype.default	4335	4299	1.01:1
TooltipMinimalPerf.default	1151	1136	1.01:1
AttachmentSlotsPerf.default	1210	1209	1:1
CheckboxMinimalPerf.default	3102	3089	1:1
EmbedMinimalPerf.default	4679	4695	1:1
LoaderMinimalPerf.default	802	803	1:1
TreeMinimalPerf.default	937	940	1:1
VideoMinimalPerf.default	764	765	1:1
ButtonOverridesMissPerf.default	1920	1944	0.99:1
DatepickerMinimalPerf.default	6256	6296	0.99:1
GridMinimalPerf.default	398	401	0.99:1
ListCommonPerf.default	744	749	0.99:1
ProviderMergeThemesPerf.default	1814	1835	0.99:1
ReactionMinimalPerf.default	450	454	0.99:1
ToolbarMinimalPerf.default	1115	1127	0.99:1
HeaderMinimalPerf.default	419	426	0.98:1
TableMinimalPerf.default	474	487	0.97:1
AccordionMinimalPerf.default	174	191	0.91:1

packages/react-utilities/src/compose/resolveShorthand.ts

packages/react-utilities/src/compose/types.ts

packages/react-utilities/src/compose/getSlots.ts

packages/react-utilities/src/compose/getSlots.test.tsx

packages/react-utilities/src/compose/getSlots.ts

packages/react-utilities/src/compose/types.ts

codesandbox-ci · 2021-06-30T15:43:12Z

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 4d0eac3:

Sandbox	Source
Fluent UI Button	Configuration
codesandbox-react-template	Configuration
codesandbox-react-northstar-template	Configuration

ecraig12345 · 2021-06-30T19:41:31Z

@bsunderhus Could you add an example of what using the new APIs looks like in one component?

bsunderhus · 2021-06-30T20:08:05Z

@bsunderhus Could you add an example of what using the new APIs looks like in one component?

WDYM by adding an example @ecraig12345 ?! I've implemented this functionality for both react-button and react-accordion in this PR as a good example case.

packages/react-utilities/src/compose/getSlots.test.tsx

Co-authored-by: ling1726 <lingfangao@hotmail.com>

msft-fluent-ui-bot · 2021-07-02T23:18:27Z

🎉@fluentui/react-tabster@v9.0.0-alpha.39 has been released which incorporates this pull request.:tada:

Handy links:

msft-fluent-ui-bot · 2021-07-02T23:18:30Z

🎉@fluentui/react-provider@v9.0.0-alpha.52 has been released which incorporates this pull request.:tada:

Handy links:

msft-fluent-ui-bot · 2021-07-02T23:18:33Z

🎉@fluentui/react-positioning@v9.0.0-alpha.32 has been released which incorporates this pull request.:tada:

Handy links:

msft-fluent-ui-bot · 2021-07-02T23:18:39Z

🎉@fluentui/react-portal@v9.0.0-alpha.26 has been released which incorporates this pull request.:tada:

Handy links:

msft-fluent-ui-bot · 2021-07-02T23:18:42Z

🎉@fluentui/react-popover@v9.0.0-alpha.14 has been released which incorporates this pull request.:tada:

Handy links:

msft-fluent-ui-bot · 2021-07-02T23:18:48Z

🎉@fluentui/react-make-styles@v9.0.0-alpha.46 has been released which incorporates this pull request.:tada:

Handy links:

msft-fluent-ui-bot · 2021-07-02T23:18:57Z

🎉@fluentui/react-label@v9.0.0-alpha.14 has been released which incorporates this pull request.:tada:

Handy links:

msft-fluent-ui-bot · 2021-07-02T23:19:06Z

🎉@fluentui/react-examples@v8.33.4 has been released which incorporates this pull request.:tada:

Handy links:

msft-fluent-ui-bot · 2021-07-02T23:19:12Z

🎉@fluentui/react-context-selector@v9.0.0-alpha.15 has been released which incorporates this pull request.:tada:

Handy links:

msft-fluent-ui-bot · 2021-07-02T23:19:18Z

🎉@fluentui/react-checkbox@v9.0.0-alpha.7 has been released which incorporates this pull request.:tada:

Handy links:

msft-fluent-ui-bot · 2021-07-02T23:19:30Z

🎉@fluentui/react-aria@v9.0.0-alpha.7 has been released which incorporates this pull request.:tada:

Handy links:

msft-fluent-ui-bot · 2021-07-02T23:19:39Z

🎉@fluentui/react-utilities@v9.0.0-alpha.31 has been released which incorporates this pull request.:tada:

Handy links:

msft-fluent-ui-bot · 2021-07-02T23:19:42Z

🎉@fluentui/public-docsite-resources@v8.1.37 has been released which incorporates this pull request.:tada:

Handy links:

msft-fluent-ui-bot · 2021-07-02T23:19:45Z

🎉@fluentui/jest-serializer-make-styles@v9.0.0-alpha.22 has been released which incorporates this pull request.:tada:

Handy links:

packages/react-accordion/src/components/Accordion/Accordion.types.ts

layershifter · 2021-07-07T12:27:08Z

packages/react-utilities/src/compose/getSlots.ts

+      const { as, children } = typedState[name];
+
+      const slot = getSlot(typedState.components ? typedState.components[name] : undefined, as) as Slots[typeof name];


According to the RFC (https://github.com/microsoft/fluentui/blob/master/rfcs/convergence/simplify-prop-merging.md#do-not-support-as-prop-for-slots-that-render-native-dom-elements) this part is redundant. It should be just:

const slot = typedState.components[name] || 'div'

layershifter · 2021-07-07T12:28:59Z

packages/react-utilities/src/compose/getSlots.ts

+        slotProps[name] =
+          typeof slots[name] === 'string'
+            ? (omit(typedState[name], ['as']) as ComponentState<SlotProps>[keyof SlotProps])
+            : typedState[name];


We also don't need to filter out as prop as it should not be a thing for native elements.

layershifter · 2021-07-07T12:31:07Z

packages/react-utilities/src/compose/types.ts

+  | ObjectShorthandProps<Props>;
+
+export type ObjectShorthandProps<Props extends { children?: React.ReactNode } = {}> = Props &
+  Pick<ComponentProps, 'as'> & {


https://github.com/microsoft/fluentui/blob/master/rfcs/convergence/simplify-prop-merging.md#do-not-support-as-prop-for-slots-that-render-native-dom-elements

as should be added only if the component itself supports it, we can remove Pick<> from there

bsunderhus added the Status: Blocked Resolution blocked by another issue label Jun 25, 2021

bsunderhus requested a review from layershifter June 25, 2021 14:31

bsunderhus self-assigned this Jun 25, 2021

bsunderhus requested a review from a team June 25, 2021 14:42

bsunderhus force-pushed the simplify-prop-merging-implementation branch 3 times, most recently from f0646fb to c2088cb Compare June 28, 2021 08:05

layershifter reviewed Jun 28, 2021

View reviewed changes

bsunderhus requested a review from layershifter June 28, 2021 21:06

ling1726 reviewed Jun 29, 2021

View reviewed changes

packages/react-utilities/src/compose/getSlots.test.tsx Outdated Show resolved Hide resolved

ling1726 reviewed Jun 29, 2021

View reviewed changes

packages/react-utilities/src/compose/getSlots.ts Outdated Show resolved Hide resolved

ling1726 reviewed Jun 29, 2021

View reviewed changes

packages/react-utilities/src/compose/types.ts Outdated Show resolved Hide resolved

ling1726 reviewed Jun 29, 2021

View reviewed changes

packages/react-utilities/src/compose/types.ts Outdated Show resolved Hide resolved

bsunderhus force-pushed the simplify-prop-merging-implementation branch 5 times, most recently from 3fdb70c to 5202cb6 Compare June 30, 2021 08:06

bsunderhus mentioned this pull request Jun 30, 2021

react-button 1st rule of ARIA refactor #18582

Closed

1 task

bsunderhus force-pushed the simplify-prop-merging-implementation branch 4 times, most recently from 7e3a28b to ed72839 Compare June 30, 2021 15:31

bsunderhus mentioned this pull request Jun 30, 2021

RFC: Simplify prop merging with slots #18642

Merged

bsunderhus requested a review from ling1726 June 30, 2021 20:08

bsunderhus requested review from a team and removed request for a team, behowell, khmakoto and sopranopillow July 2, 2021 08:27

ling1726 reviewed Jul 2, 2021

View reviewed changes

packages/react-utilities/src/compose/getSlots.test.tsx Outdated Show resolved Hide resolved

bsunderhus and others added 2 commits July 2, 2021 10:33

Update packages/react-utilities/src/compose/getSlots.test.tsx

4b417d0

Co-authored-by: ling1726 <lingfangao@hotmail.com>

Update packages/react-utilities/src/compose/getSlots.test.tsx

4d0eac3

Co-authored-by: ling1726 <lingfangao@hotmail.com>

ling1726 approved these changes Jul 2, 2021

View reviewed changes

bsunderhus merged commit 6c37a1c into microsoft:master Jul 2, 2021

bsunderhus deleted the simplify-prop-merging-implementation branch July 2, 2021 12:06

This was referenced Jul 2, 2021

react-accordion - Simplify prop merging #18813

Merged

react-button - Simplify prop merging #18814

Merged

layershifter reviewed Jul 7, 2021

View reviewed changes

bsunderhus mentioned this pull request Jul 8, 2021

fix(react-utilities): simplify prop merging in getSlots() #18861

Merged

1 task

		const { as, children } = typedState[name];

		const slot = getSlot(typedState.components ? typedState.components[name] : undefined, as) as Slots[typeof name];

Conversation

bsunderhus commented Jun 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request checklist

Description of changes

PREVIEW 📃

Problems so far in this implementation

Uh oh!

size-auditor bot commented Jun 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Asset size changes

Uh oh!

fabricteam commented Jun 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Perf Analysis (@fluentui/react)

Perf Analysis (@fluentui/react-northstar)

Potential regressions comparing to master

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codesandbox-ci bot commented Jun 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ecraig12345 commented Jun 30, 2021

Uh oh!

bsunderhus commented Jun 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

msft-fluent-ui-bot commented Jul 2, 2021

Uh oh!

msft-fluent-ui-bot commented Jul 2, 2021

Uh oh!

msft-fluent-ui-bot commented Jul 2, 2021

Uh oh!

msft-fluent-ui-bot commented Jul 2, 2021

Uh oh!

msft-fluent-ui-bot commented Jul 2, 2021

Uh oh!

msft-fluent-ui-bot commented Jul 2, 2021

Uh oh!

msft-fluent-ui-bot commented Jul 2, 2021

Uh oh!

msft-fluent-ui-bot commented Jul 2, 2021

Uh oh!

msft-fluent-ui-bot commented Jul 2, 2021

Uh oh!

msft-fluent-ui-bot commented Jul 2, 2021

Uh oh!

msft-fluent-ui-bot commented Jul 2, 2021

Uh oh!

msft-fluent-ui-bot commented Jul 2, 2021

Uh oh!

msft-fluent-ui-bot commented Jul 2, 2021

Uh oh!

msft-fluent-ui-bot commented Jul 2, 2021

Uh oh!

Uh oh!

layershifter Jul 7, 2021

Choose a reason for hiding this comment

Uh oh!

layershifter Jul 7, 2021

Choose a reason for hiding this comment

Uh oh!

layershifter Jul 7, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

bsunderhus commented Jun 25, 2021 •

edited

Loading

size-auditor bot commented Jun 25, 2021 •

edited

Loading

fabricteam commented Jun 25, 2021 •

edited

Loading

Perf Analysis (`@fluentui/react`)

Perf Analysis (`@fluentui/react-northstar`)

codesandbox-ci bot commented Jun 30, 2021 •

edited

Loading

bsunderhus commented Jun 30, 2021 •

edited

Loading