Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs(rfcs): type-checking-perf-improvements #30514

Merged
merged 2 commits into from
Mar 6, 2024

Conversation

Hotell
Copy link
Contributor

@Hotell Hotell commented Feb 9, 2024

@github-actions github-actions bot added the Type: RFC Request for Feedback label Feb 9, 2024
@Hotell Hotell changed the title Rfc/v9 project domain changes docs(rfcs): add type-checking-perf-improvements Feb 9, 2024
@fabricteam
Copy link
Collaborator

fabricteam commented Feb 9, 2024

📊 Bundle size report

🤖 This report was generated against b072525b88f147ed988efd2e17a5bc9d4d1238a0

@fabricteam
Copy link
Collaborator

fabricteam commented Feb 9, 2024

Perf Analysis (@fluentui/react-components)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 621 650 5000
Button mount 319 300 5000
Field mount 1135 1127 5000
FluentProvider mount 694 681 5000
FluentProviderWithTheme mount 82 80 10
FluentProviderWithTheme virtual-rerender 69 60 10
FluentProviderWithTheme virtual-rerender-with-unmount 75 82 10
MakeStyles mount 860 868 50000
Persona mount 1794 1762 5000
SpinButton mount 1434 1385 5000

@fabricteam
Copy link
Collaborator

fabricteam commented Feb 9, 2024

🕵 fluentuiv8 No visual regressions between this PR and main

@fabricteam
Copy link
Collaborator

fabricteam commented Feb 9, 2024

Perf Analysis (@fluentui/react-northstar)

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
AvatarMinimalPerf.default 120 100 1.2:1
TreeWith60ListItems.default 94 84 1.12:1
AttachmentMinimalPerf.default 85 77 1.1:1
CardMinimalPerf.default 320 295 1.08:1
RefMinimalPerf.default 114 106 1.08:1
RosterPerf.default 1642 1540 1.07:1
DialogMinimalPerf.default 458 433 1.06:1
ButtonSlotsPerf.default 322 306 1.05:1
DividerMinimalPerf.default 208 199 1.05:1
FormMinimalPerf.default 225 215 1.05:1
ImageMinimalPerf.default 219 208 1.05:1
LayoutMinimalPerf.default 203 194 1.05:1
TextAreaMinimalPerf.default 294 279 1.05:1
BoxMinimalPerf.default 195 188 1.04:1
GridMinimalPerf.default 196 189 1.04:1
ListWith60ListItems.default 370 355 1.04:1
HeaderMinimalPerf.default 206 200 1.03:1
ItemLayoutMinimalPerf.default 731 710 1.03:1
ListNestedPerf.default 337 326 1.03:1
SegmentMinimalPerf.default 207 200 1.03:1
SkeletonMinimalPerf.default 211 204 1.03:1
TooltipMinimalPerf.default 1286 1247 1.03:1
AttachmentSlotsPerf.default 669 654 1.02:1
CarouselMinimalPerf.default 263 257 1.02:1
ChatMinimalPerf.default 444 436 1.02:1
FlexMinimalPerf.default 160 157 1.02:1
ProviderMinimalPerf.default 205 200 1.02:1
ReactionMinimalPerf.default 214 210 1.02:1
SplitButtonMinimalPerf.default 2307 2261 1.02:1
StatusMinimalPerf.default 397 389 1.02:1
TableMinimalPerf.default 238 234 1.02:1
CustomToolbarPrototype.default 1476 1441 1.02:1
VideoMinimalPerf.default 444 435 1.02:1
CheckboxMinimalPerf.default 1153 1144 1.01:1
DropdownManyItemsPerf.default 391 388 1.01:1
EmbedMinimalPerf.default 1877 1866 1.01:1
PortalMinimalPerf.default 87 86 1.01:1
TableManyItemsPerf.default 1119 1109 1.01:1
TreeMinimalPerf.default 494 487 1.01:1
ChatDuplicateMessagesPerf.default 150 150 1:1
DropdownMinimalPerf.default 1451 1446 1:1
ListCommonPerf.default 394 394 1:1
MenuButtonMinimalPerf.default 954 957 1:1
PopupMinimalPerf.default 350 349 1:1
ProviderMergeThemesPerf.default 651 649 1:1
AlertMinimalPerf.default 153 154 0.99:1
ButtonMinimalPerf.default 83 84 0.99:1
ChatWithPopoverPerf.default 195 196 0.99:1
DatepickerMinimalPerf.default 3588 3641 0.99:1
HeaderSlotsPerf.default 477 480 0.99:1
ListMinimalPerf.default 305 308 0.99:1
LoaderMinimalPerf.default 202 205 0.99:1
SliderMinimalPerf.default 747 756 0.99:1
IconMinimalPerf.default 390 395 0.99:1
TextMinimalPerf.default 188 189 0.99:1
AccordionMinimalPerf.default 79 81 0.98:1
InputMinimalPerf.default 551 564 0.98:1
MenuMinimalPerf.default 507 515 0.98:1
RadioGroupMinimalPerf.default 251 257 0.98:1
ToolbarMinimalPerf.default 539 549 0.98:1
AnimationMinimalPerf.default 285 295 0.97:1
LabelMinimalPerf.default 213 232 0.92:1
ButtonOverridesMissPerf.default 632 695 0.91:1

@fabricteam
Copy link
Collaborator

fabricteam commented Feb 9, 2024

Perf Analysis (@fluentui/react)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
BaseButton mount 640 621 5000
Breadcrumb mount 1706 1659 1000
Checkbox mount 1705 1693 5000
CheckboxBase mount 1487 1446 5000
ChoiceGroup mount 2960 2984 5000
ComboBox mount 654 641 1000
CommandBar mount 6204 6219 1000
ContextualMenu mount 12086 12386 1000
DefaultButton mount 746 743 5000
DetailsRow mount 2187 2216 5000
DetailsRowFast mount 2216 2167 5000
DetailsRowNoStyles mount 2078 2009 5000
Dialog mount 2743 2842 1000
DocumentCardTitle mount 213 233 1000
Dropdown mount 1990 2009 5000
FocusTrapZone mount 1141 1145 5000
FocusZone mount 1079 1069 5000
GroupedList mount 41557 47929 2
GroupedList virtual-rerender 17820 20034 2
GroupedList virtual-rerender-with-unmount 51081 50985 2
GroupedListV2 mount 217 221 2
GroupedListV2 virtual-rerender 213 205 2
GroupedListV2 virtual-rerender-with-unmount 229 224 2
IconButton mount 1075 1094 5000
Label mount 349 340 5000
Layer mount 2752 2753 5000
Link mount 391 386 5000
MenuButton mount 937 960 5000
MessageBar mount 21861 21410 5000
Nav mount 1997 1950 1000
OverflowSet mount 777 759 5000
Panel mount 2080 1741 1000
Persona mount 730 758 1000
Pivot mount 864 875 1000
PrimaryButton mount 841 825 5000
Rating mount 4575 4579 5000
SearchBox mount 878 886 5000
Shimmer mount 1840 1857 5000
Slider mount 1295 1372 5000
SpinButton mount 2885 2830 5000
Spinner mount 384 390 5000
SplitButton mount 1815 1862 5000
Stack mount 401 391 5000
StackWithIntrinsicChildren mount 848 844 5000
StackWithTextChildren mount 2563 2568 5000
SwatchColorPicker mount 6228 6154 5000
TagPicker mount 1467 1448 5000
Text mount 365 378 5000
TextField mount 928 957 5000
ThemeProvider mount 822 832 5000
ThemeProvider virtual-rerender 587 604 5000
ThemeProvider virtual-rerender-with-unmount 1272 1264 5000
Toggle mount 607 607 5000
buttonNative mount 192 182 5000

@fabricteam
Copy link
Collaborator

fabricteam commented Feb 9, 2024

🕵 fluentuiv9 No visual regressions between this PR and main

@Hotell Hotell changed the title docs(rfcs): add type-checking-perf-improvements docs(rfcs): type-checking-perf-improvements Feb 9, 2024
@Hotell Hotell force-pushed the rfc/v9-project-domain-changes branch from aaab11f to c59ff3c Compare February 14, 2024 18:33
@fabricteam
Copy link
Collaborator

🕵 FluentUIV0 No visual regressions between this PR and main

@Hotell Hotell force-pushed the rfc/v9-project-domain-changes branch 2 times, most recently from 3f59dca to bacbe3b Compare February 21, 2024 15:53
@Hotell Hotell marked this pull request as ready for review February 21, 2024 15:56
@Hotell Hotell requested a review from a team as a code owner February 21, 2024 15:56
Copy link

codesandbox-ci bot commented Feb 21, 2024

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.

@Hotell Hotell requested review from a team February 21, 2024 17:29
Copy link
Member

@TristanWatanabe TristanWatanabe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to the get rid of circular dependencies solution, specifically separating stories into their own packages. It's the most maintainable and scalable out of all proposed solutions IMO.

And as far as the flat or nested folder structure, I'd lean more towards a nested folder structure since that seems cleaner and less confusing. Cluttering the packages/react-components subfolder with a bunch of -stories (and eventually -vr-tests) would really cause that folder to balloon in size rather quickly and could be overwhelming for any external contributors in the future. Only downside is that it'll cause the most amount of churn in the repo with all the file movements (both stories and library implementation) but that would only be a short term downside

@tudorpopams tudorpopams self-requested a review February 27, 2024 13:03
@layershifter
Copy link
Member

From the RFC, I see that there are 4 options currently:

  • 1: hack fix the script/configs
  • ⁠2.1: get rid of circular dependencies: stop importing from react-components suite within stories
  • ⁠2.2.1: get rid of circular dependencies: move code to separate packages + flat structure
  • ⁠2.2.2: get rid of circular dependencies: move code to separate packages + nested structure

@spmonahan @marcosmoura I noticed that you have already approved the RFC, can you please comment on the preferred option?

@spmonahan
Copy link
Contributor

From the RFC, I see that there are 4 options currently:

  • 1: hack fix the script/configs
  • ⁠2.1: get rid of circular dependencies: stop importing from react-components suite within stories
  • ⁠2.2.1: get rid of circular dependencies: move code to separate packages + flat structure
  • ⁠2.2.2: get rid of circular dependencies: move code to separate packages + nested structure

@spmonahan @marcosmoura I noticed that you have already approved the RFC, can you please comment on the preferred option?

2.2.2. My understanding from our meeting was that this is the recommended option.

@layershifter
Copy link
Member

  • ⁠2.2.2: get rid of circular dependencies: move code to separate packages + nested structure

In the offline conversation with @ling1726 we agreed to propose to go with this option as it contains less magic compared to 2.1.

@marcosmoura
Copy link
Contributor

From the RFC, I see that there are 4 options currently:

  • 1: hack fix the script/configs
  • ⁠2.1: get rid of circular dependencies: stop importing from react-components suite within stories
  • ⁠2.2.1: get rid of circular dependencies: move code to separate packages + flat structure
  • ⁠2.2.2: get rid of circular dependencies: move code to separate packages + nested structure

@spmonahan @marcosmoura I noticed that you have already approved the RFC, can you please comment on the preferred option?

I spoke with @Hotell offline about it, but I prefer the "get rid of circular dependencies - nested folder structure" option. It will make it super well organized, clear separation of dependencies and it'll make it possible to use components stories somewhere else (if we need to) in a more predictive way.

@Hotell Hotell force-pushed the rfc/v9-project-domain-changes branch from ffa988a to d0dd436 Compare March 6, 2024 15:44
@Hotell Hotell enabled auto-merge (squash) March 6, 2024 15:44
@Hotell Hotell merged commit 09cc76c into microsoft:master Mar 6, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: RFC Request for Feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants