chore(react-utilities): Deletes descendants API #19189

bsunderhus · 2021-07-29T17:40:36Z

Pull request checklist

Include a change request file using $ yarn change

Description of changes

Deletes descendants API
Refactor react-accordion to use explicit values

Breaking change in `react-accordion` API! ⚠️

Spec has been updated accordingly with the following changes:

Accordion API: migrate from index to openItems and defaultIndex to defaultOpenItems
AccordionItem: adds required prop value that will be used to identify the item

Motivations

Biggest motivation for this breaking change is performance. Descendants API has proved to be more cumbersome than beneficial. It introduced a huge number of re-renders that increased linearly by the number of descendants provided.

By opting for a more explicit index declaration no re-rendering is necessary.
The biggest down side is that it becomes user responsibility to define AccordionItem value.

fabricteam · 2021-07-29T17:43:19Z

📊 Bundle size report

Package & Exports	Baseline (minified/GZIP)	PR	Change
`react-accordion` Accordion (including children components)	`72.65 kB` `21.364 kB`	`75.578 kB` `22.287 kB`	`-2.928 kB` `-923 B`
`react-components` react-components: Accordion, Button, FluentProvider, Image, Menu, Popover	`174.117 kB` `49.225 kB`	`176.958 kB` `50.08 kB`	`-2.841 kB` `-855 B`

Unchanged fixtures

Package & Exports	Size (minified/GZIP)
`react-avatar` Avatar	`56.558 kB` `15.66 kB`
`react-badge` Badge	`24.343 kB` `7.165 kB`
`react-badge` CounterBadge	`27.156 kB` `7.851 kB`
`react-badge` PresenseBadge	`237 B` `177 B`
`react-button` Button	`24.934 kB` `8.001 kB`
`react-button` CompoundButton	`30.226 kB` `8.878 kB`
`react-button` MenuButton	`26.521 kB` `8.509 kB`
`react-button` ToggleButton	`34.531 kB` `8.637 kB`
`react-components` react-components: FluentProvider & webLightTheme	`36.237 kB` `11.596 kB`
`react-divider` Divider	`15.889 kB` `5.747 kB`
`react-image` Image	`10.642 kB` `4.264 kB`
`react-label` Label	`9.397 kB` `3.839 kB`
`react-link` Link	`14.715 kB` `6.012 kB`
`react-make-styles` makeStaticStyles (runtime)	`7.59 kB` `3.321 kB`
`react-make-styles` makeStyles + mergeClasses (runtime)	`22.135 kB` `8.356 kB`
`react-make-styles` makeStyles + mergeClasses (build time)	`2.557 kB` `1.202 kB`
`react-menu` Menu (including children components)	`114.61 kB` `34.554 kB`
`react-menu` Menu (including selectable components)	`116.71 kB` `34.824 kB`
`react-popover` Popover	`124.181 kB` `36.121 kB`
`react-portal` Portal	`7.78 kB` `2.672 kB`
`react-positioning` usePopper	`23.157 kB` `7.942 kB`
`react-provider` FluentProvider	`16.235 kB` `5.972 kB`
`react-tooltip` Tooltip	`45.281 kB` `15.45 kB`
`react-utilities` SSRProvider	`213 B` `170 B`

_{🤖 This report was generated against 4935a7fc471066f168c3e1ef0801035ea489fde5}

codesandbox-ci · 2021-07-29T17:47:27Z

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 4250563:

Sandbox	Source
Fluent UI React Starter	Configuration

size-auditor · 2021-07-29T17:50:05Z

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: 4935a7fc471066f168c3e1ef0801035ea489fde5 (build)

fabricteam · 2021-07-30T17:04:34Z

Perf Analysis (`@fluentui/react`)

No significant results to display.

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations
Avatar	mount	839	857	5000
BaseButton	mount	840	834	5000
Breadcrumb	mount	2505	2470	1000
ButtonNext	mount	406	426	5000
Checkbox	mount	1447	1420	5000
CheckboxBase	mount	1215	1224	5000
ChoiceGroup	mount	4435	4483	5000
ComboBox	mount	931	918	1000
CommandBar	mount	9584	9577	1000
ContextualMenu	mount	5749	5876	1000
DefaultButton	mount	1041	1048	5000
DetailsRow	mount	3647	3534	5000
DetailsRowFast	mount	3541	3616	5000
DetailsRowNoStyles	mount	3279	3313	5000
Dialog	mount	2045	2033	1000
DocumentCardTitle	mount	139	136	1000
Dropdown	mount	3054	3019	5000
FluentProviderNext	mount	7171	7043	5000
FocusTrapZone	mount	1640	1668	5000
FocusZone	mount	1710	1707	5000
IconButton	mount	1644	1624	5000
Label	mount	314	318	5000
Layer	mount	1678	1646	5000
Link	mount	441	437	5000
MakeStyles	mount	1707	1729	50000
MenuButton	mount	1382	1381	5000
MessageBar	mount	1921	1906	5000
Nav	mount	3050	3079	1000
OverflowSet	mount	967	972	5000
Panel	mount	1946	1960	1000
Persona	mount	778	781	1000
Pivot	mount	1313	1354	1000
PrimaryButton	mount	1202	1202	5000
Rating	mount	7170	7192	5000
SearchBox	mount	1242	1236	5000
Shimmer	mount	2376	2383	5000
Slider	mount	1878	1873	5000
SpinButton	mount	4685	4957	5000
Spinner	mount	393	405	5000
SplitButton	mount	2974	2952	5000
Stack	mount	467	474	5000
StackWithIntrinsicChildren	mount	1433	1452	5000
StackWithTextChildren	mount	4176	4262	5000
SwatchColorPicker	mount	9596	9522	5000
Tabs	mount	1294	1315	1000
TagPicker	mount	2412	2460	5000
TeachingBubble	mount	11283	11338	5000
Text	mount	395	383	5000
TextField	mount	1264	1282	5000
ThemeProvider	mount	1118	1103	5000
ThemeProvider	virtual-rerender	565	563	5000
Toggle	mount	754	739	5000
buttonNative	mount	107	105	5000

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

Perf tests with no regressions

Scenario	Current PR Ticks	Baseline Ticks	Ratio
AttachmentMinimalPerf.default	152	141	1.08:1
HeaderSlotsPerf.default	706	673	1.05:1
ListWith60ListItems.default	604	577	1.05:1
ReactionMinimalPerf.default	351	335	1.05:1
AnimationMinimalPerf.default	394	380	1.04:1
ChatDuplicateMessagesPerf.default	285	274	1.04:1
ChatMinimalPerf.default	633	611	1.04:1
ButtonSlotsPerf.default	532	515	1.03:1
DatepickerMinimalPerf.default	5209	5069	1.03:1
ItemLayoutMinimalPerf.default	1156	1122	1.03:1
ListCommonPerf.default	594	577	1.03:1
SkeletonMinimalPerf.default	328	319	1.03:1
TableManyItemsPerf.default	1805	1756	1.03:1
DialogMinimalPerf.default	708	697	1.02:1
HeaderMinimalPerf.default	331	325	1.02:1
LayoutMinimalPerf.default	352	346	1.02:1
MenuMinimalPerf.default	801	782	1.02:1
PortalMinimalPerf.default	167	164	1.02:1
RefMinimalPerf.default	221	217	1.02:1
TreeWith60ListItems.default	164	161	1.02:1
VideoMinimalPerf.default	598	587	1.02:1
AttachmentSlotsPerf.default	1011	1003	1.01:1
BoxMinimalPerf.default	333	330	1.01:1
CardMinimalPerf.default	512	505	1.01:1
ChatWithPopoverPerf.default	339	334	1.01:1
CheckboxMinimalPerf.default	2656	2618	1.01:1
DropdownManyItemsPerf.default	632	627	1.01:1
EmbedMinimalPerf.default	3910	3858	1.01:1
FormMinimalPerf.default	370	367	1.01:1
GridMinimalPerf.default	319	315	1.01:1
ListMinimalPerf.default	478	471	1.01:1
ListNestedPerf.default	519	516	1.01:1
LoaderMinimalPerf.default	652	643	1.01:1
MenuButtonMinimalPerf.default	1539	1523	1.01:1
PopupMinimalPerf.default	568	564	1.01:1
ProviderMinimalPerf.default	925	915	1.01:1
SplitButtonMinimalPerf.default	3577	3557	1.01:1
AlertMinimalPerf.default	252	253	1:1
AvatarMinimalPerf.default	183	183	1:1
ButtonOverridesMissPerf.default	1631	1638	1:1
InputMinimalPerf.default	1208	1209	1:1
SliderMinimalPerf.default	1472	1468	1:1
StatusMinimalPerf.default	628	628	1:1
TextAreaMinimalPerf.default	474	472	1:1
CustomToolbarPrototype.default	3610	3610	1:1
ToolbarMinimalPerf.default	865	867	1:1
TooltipMinimalPerf.default	937	934	1:1
CarouselMinimalPerf.default	445	449	0.99:1
DropdownMinimalPerf.default	2923	2943	0.99:1
ImageMinimalPerf.default	341	343	0.99:1
RadioGroupMinimalPerf.default	416	420	0.99:1
IconMinimalPerf.default	580	584	0.99:1
TableMinimalPerf.default	374	379	0.99:1
ButtonMinimalPerf.default	153	156	0.98:1
DividerMinimalPerf.default	324	330	0.98:1
FlexMinimalPerf.default	260	266	0.98:1
ProviderMergeThemesPerf.default	1560	1598	0.98:1
TextMinimalPerf.default	310	317	0.98:1
TreeMinimalPerf.default	747	759	0.98:1
AccordionMinimalPerf.default	139	144	0.97:1
LabelMinimalPerf.default	353	364	0.97:1
SegmentMinimalPerf.default	314	326	0.96:1
RosterPerf.default	1047	1100	0.95:1

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

packages/react-utilities/src/descendants/descendants.ts

layershifter

Overall it's a nice idea, but in the current state it will not work with concurrent mode. It's one of reasons why other implementations are using DOM order as with concurrent mode "rendering order" != "DOM order".

Even currently, it fails in Strict mode: https://codesandbox.io/s/icy-fire-p4vup?file=/src/index.js

packages/react-utilities/src/descendants/descendants.ts

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

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

packages/react-utilities/src/hooks/useForceUpdate.ts

layershifter

Overall, I totally support these changes 👍

But, can you please improve PR description? First of all, it's a breaking change in API, so we should describe migration (preferably via before/after). We should should add "Motivation" section to explain of decision: performance/re-renders caused by descendants usage, implementation simplicity, etc.

Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>

…ui into descendants-api-update

msft-fluent-ui-bot · 2021-08-06T07:37:56Z

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

Handy links:

msft-fluent-ui-bot · 2021-08-06T07:38:08Z

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

Handy links:

msft-fluent-ui-bot · 2021-08-06T07:38:14Z

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

Handy links:

msft-fluent-ui-bot · 2021-08-06T07:38:56Z

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

Handy links:

bsunderhus requested review from layershifter and ling1726 July 29, 2021 17:40

bsunderhus self-assigned this Jul 29, 2021

bsunderhus marked this pull request as ready for review July 30, 2021 16:43

bsunderhus requested a review from a team as a code owner July 30, 2021 16:43

ling1726 reviewed Jul 30, 2021

View reviewed changes

packages/react-accordion/src/components/Accordion/Accordion.types.ts Outdated Show resolved Hide resolved

ling1726 reviewed Jul 30, 2021

View reviewed changes

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

ling1726 reviewed Jul 30, 2021

View reviewed changes

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

ling1726 reviewed Jul 30, 2021

View reviewed changes

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

layershifter reviewed Aug 2, 2021

View reviewed changes

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

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

bsunderhus force-pushed the descendants-api-update branch 2 times, most recently from a6d0570 to b73713b Compare August 3, 2021 08:47

bsunderhus closed this Aug 3, 2021

bsunderhus reopened this Aug 3, 2021

bsunderhus force-pushed the descendants-api-update branch 2 times, most recently from feacdcb to 34cc81d Compare August 3, 2021 12:05

bsunderhus and others added 10 commits August 4, 2021 10:36

Simplifies descendants API

08bc2d6

Updates accordion descendants

4952d23

Change files

e0b3982

Update API

ff11189

Update packages/react-accordion/src/components/Accordion/Accordion.ty…

bf8643d

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

Update packages/react-utilities/src/descendants/descendants.ts

c57d289

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

Updates forceUpdate & comments

b92d1d0

Updates API

bf2efb5

Updates API

c9d9c4d

Makes support for concurrent mode

fca1b0b

bsunderhus added 2 commits August 4, 2021 10:36

Updates API

a2bffdd

Updates API

2a3f893

bsunderhus force-pushed the descendants-api-update branch from 34cc81d to 2a3f893 Compare August 4, 2021 08:36

bsunderhus changed the title ~~chore(react-utilities): Simplifies descendants API~~ chore(react-utilities): Deletes descendants API Aug 4, 2021

Deletes descendants API

2aac65e

bsunderhus force-pushed the descendants-api-update branch from 9d6daa5 to 2aac65e Compare August 4, 2021 13:24

Updates vr-tests

2cce0a4

layershifter reviewed Aug 5, 2021

View reviewed changes

packages/react-utilities/src/hooks/useForceUpdate.ts Outdated Show resolved Hide resolved

layershifter approved these changes Aug 5, 2021

View reviewed changes

bsunderhus and others added 4 commits August 5, 2021 16:03

Update packages/react-utilities/src/hooks/useForceUpdate.ts

8bee018

Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>

Updates Spec

9b085b4

Merge branch 'descendants-api-update' of github.com:bsunderhus/fluent…

b93392a

…ui into descendants-api-update

Updates API

4250563

bsunderhus merged commit 9ac8b04 into microsoft:master Aug 5, 2021

bsunderhus deleted the descendants-api-update branch August 5, 2021 15:23

chore(react-utilities): Deletes descendants API #19189

chore(react-utilities): Deletes descendants API #19189

Uh oh!

Conversation

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

Uh oh!

Pull request checklist

Description of changes

Breaking change in react-accordion API! ⚠️

Motivations

Uh oh!

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

Uh oh!

📊 Bundle size report

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Asset size changes

Uh oh!

fabricteam commented Jul 30, 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)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

layershifter left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

layershifter left a comment

Choose a reason for hiding this comment

Uh oh!

msft-fluent-ui-bot commented Aug 6, 2021

Uh oh!

msft-fluent-ui-bot commented Aug 6, 2021

Uh oh!

msft-fluent-ui-bot commented Aug 6, 2021

Uh oh!

msft-fluent-ui-bot commented Aug 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bsunderhus commented Jul 29, 2021 •

edited

Loading

Breaking change in `react-accordion` API! ⚠️

fabricteam commented Jul 29, 2021 •

edited

Loading

codesandbox-ci bot commented Jul 29, 2021 •

edited

Loading

size-auditor bot commented Jul 29, 2021 •

edited

Loading

fabricteam commented Jul 30, 2021 •

edited

Loading

Perf Analysis (`@fluentui/react`)

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

layershifter left a comment •

edited

Loading