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

RFC: Stop pre-processing styles with Griffel in @fluentui/react-components #31134

Merged
merged 7 commits into from
Jul 18, 2024

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented Apr 22, 2024

Description

The RFC proposes halting the pre-processing of styles in @fluentui/react-components to address CSS clashes observed when apps are bundled into multiple separate bundles.

Two options are presented:

  • stopping pre-processing entirely, requiring consuming apps to handle it (breaking change)
  • shipping ESM output with unprocessed styles alongside pre-processed styles, maintaining backward compatibility, but increasing complexity and package size

@github-actions github-actions bot added this to the April Project Cycle Q1 2024 milestone Apr 22, 2024
@layershifter layershifter marked this pull request as ready for review April 22, 2024 12:55
@layershifter layershifter requested review from a team as code owners April 22, 2024 12:55
@layershifter layershifter added the Type: RFC Request for Feedback label Apr 22, 2024
Copy link

codesandbox-ci bot commented Apr 22, 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.

@fabricteam
Copy link
Collaborator

fabricteam commented Apr 22, 2024

Perf Analysis (@fluentui/react-components)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 620 602 5000
Button mount 307 297 5000
Field mount 1141 1086 5000
FluentProvider mount 713 706 5000
FluentProviderWithTheme mount 93 81 10
FluentProviderWithTheme virtual-rerender 32 35 10
FluentProviderWithTheme virtual-rerender-with-unmount 72 78 10
MakeStyles mount 869 884 50000
Persona mount 1762 1686 5000
SpinButton mount 1382 1408 5000
SwatchPicker mount 1673 1680 5000

@fabricteam
Copy link
Collaborator

fabricteam commented Apr 22, 2024

Perf Analysis (@fluentui/react-northstar)

⚠️ 1 potential perf regressions detected

Potential regressions comparing to master

Scenario Current PR Ticks Baseline Ticks Ratio Regression Analysis
FlexMinimalPerf.default 163 153 1.07:1 analysis
Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
AccordionMinimalPerf.default 92 77 1.19:1
AvatarMinimalPerf.default 115 103 1.12:1
ChatWithPopoverPerf.default 207 186 1.11:1
ButtonMinimalPerf.default 90 82 1.1:1
TableMinimalPerf.default 239 221 1.08:1
ButtonSlotsPerf.default 326 305 1.07:1
HeaderMinimalPerf.default 212 199 1.07:1
LabelMinimalPerf.default 226 212 1.07:1
DividerMinimalPerf.default 209 197 1.06:1
DropdownManyItemsPerf.default 404 382 1.06:1
RefMinimalPerf.default 115 108 1.06:1
InputMinimalPerf.default 552 524 1.05:1
ListNestedPerf.default 321 307 1.05:1
ListWith60ListItems.default 378 360 1.05:1
PortalMinimalPerf.default 86 82 1.05:1
CarouselMinimalPerf.default 264 253 1.04:1
RosterPerf.default 1602 1534 1.04:1
SkeletonMinimalPerf.default 199 191 1.04:1
SplitButtonMinimalPerf.default 2268 2174 1.04:1
StatusMinimalPerf.default 411 396 1.04:1
GridMinimalPerf.default 188 182 1.03:1
LayoutMinimalPerf.default 203 197 1.03:1
LoaderMinimalPerf.default 189 184 1.03:1
PopupMinimalPerf.default 347 338 1.03:1
CheckboxMinimalPerf.default 1144 1120 1.02:1
DialogMinimalPerf.default 443 435 1.02:1
EmbedMinimalPerf.default 1868 1827 1.02:1
ImageMinimalPerf.default 228 224 1.02:1
ItemLayoutMinimalPerf.default 718 706 1.02:1
MenuMinimalPerf.default 498 486 1.02:1
TextMinimalPerf.default 191 188 1.02:1
TextAreaMinimalPerf.default 287 280 1.02:1
DatepickerMinimalPerf.default 3526 3501 1.01:1
ProviderMergeThemesPerf.default 661 653 1.01:1
ProviderMinimalPerf.default 205 203 1.01:1
ReactionMinimalPerf.default 211 209 1.01:1
ToolbarMinimalPerf.default 552 548 1.01:1
TreeWith60ListItems.default 85 84 1.01:1
AlertMinimalPerf.default 155 155 1:1
ListCommonPerf.default 385 384 1:1
TreeMinimalPerf.default 486 488 1:1
AnimationMinimalPerf.default 291 294 0.99:1
AttachmentMinimalPerf.default 78 79 0.99:1
BoxMinimalPerf.default 187 188 0.99:1
CardMinimalPerf.default 301 303 0.99:1
DropdownMinimalPerf.default 1397 1410 0.99:1
ListMinimalPerf.default 299 301 0.99:1
MenuButtonMinimalPerf.default 933 944 0.99:1
SegmentMinimalPerf.default 191 192 0.99:1
IconMinimalPerf.default 388 393 0.99:1
TableManyItemsPerf.default 1107 1114 0.99:1
TooltipMinimalPerf.default 1271 1280 0.99:1
VideoMinimalPerf.default 416 421 0.99:1
ButtonOverridesMissPerf.default 633 646 0.98:1
RadioGroupMinimalPerf.default 261 265 0.98:1
CustomToolbarPrototype.default 1429 1456 0.98:1
ChatMinimalPerf.default 434 449 0.97:1
HeaderSlotsPerf.default 457 469 0.97:1
SliderMinimalPerf.default 716 738 0.97:1
ChatDuplicateMessagesPerf.default 153 159 0.96:1
AttachmentSlotsPerf.default 615 644 0.95:1
FormMinimalPerf.default 210 225 0.93:1

@fabricteam
Copy link
Collaborator

fabricteam commented Apr 22, 2024

🕵 fluentuiv8 No visual regressions between this PR and main

@fabricteam
Copy link
Collaborator

fabricteam commented Apr 22, 2024

🕵 FluentUIV0 No visual regressions between this PR and main

@fabricteam
Copy link
Collaborator

fabricteam commented Apr 22, 2024

📊 Bundle size report

✅ No changes found

@fabricteam
Copy link
Collaborator

fabricteam commented Apr 22, 2024

Perf Analysis (@fluentui/react)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
BaseButton mount 619 643 5000
Breadcrumb mount 1695 1738 1000
Checkbox mount 1715 1718 5000
CheckboxBase mount 1469 1488 5000
ChoiceGroup mount 2985 2957 5000
ComboBox mount 689 679 1000
CommandBar mount 6660 6594 1000
ContextualMenu mount 14241 14262 1000
DefaultButton mount 789 810 5000
DetailsRow mount 2225 2230 5000
DetailsRowFast mount 2273 2255 5000
DetailsRowNoStyles mount 2070 2045 5000
Dialog mount 2790 2700 1000
DocumentCardTitle mount 230 251 1000
Dropdown mount 2006 1973 5000
FocusTrapZone mount 1172 1161 5000
FocusZone mount 1073 1092 5000
GroupedList mount 42463 42574 2
GroupedList virtual-rerender 18255 20514 2
GroupedList virtual-rerender-with-unmount 51376 51904 2
GroupedListV2 mount 220 223 2
GroupedListV2 virtual-rerender 216 217 2
GroupedListV2 virtual-rerender-with-unmount 225 225 2
IconButton mount 1200 1175 5000
Label mount 362 347 5000
Layer mount 2779 2772 5000
Link mount 410 406 5000
MenuButton mount 982 1038 5000
MessageBar mount 21584 21481 5000
Nav mount 2060 2073 1000
OverflowSet mount 782 801 5000
Panel mount 1777 1856 1000
Persona mount 758 765 1000
Pivot mount 905 885 1000
PrimaryButton mount 926 944 5000
Rating mount 4748 4753 5000
SearchBox mount 948 945 5000
Shimmer mount 1953 1926 5000
Slider mount 1338 1348 5000
SpinButton mount 2990 3083 5000
Spinner mount 393 383 5000
SplitButton mount 1893 1883 5000
Stack mount 431 424 5000
StackWithIntrinsicChildren mount 871 904 5000
StackWithTextChildren mount 2798 2763 5000
SwatchColorPicker mount 6455 6439 5000
TagPicker mount 1499 1485 5000
Text mount 389 390 5000
TextField mount 966 958 5000
ThemeProvider mount 852 854 5000
ThemeProvider virtual-rerender 592 590 5000
ThemeProvider virtual-rerender-with-unmount 1285 1297 5000
Toggle mount 623 629 5000
buttonNative mount 186 194 5000

@tudorpopams tudorpopams requested a review from Hotell April 23, 2024 12:03
Copy link
Contributor

@Hotell Hotell left a comment

Choose a reason for hiding this comment

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

1st take POV:

None of the proposed solutions seems be good enough for maintenance, scalability, and DX/UX for consumers.

IFF we would need to choose ASAP I'm inclined more to the 1st option, based on data on how big that impact might actually be + taking into consideration that we can incorporate those not very user friendly loader overrides in some kind of GriffelPlugin to ease the pain in userland.

@github-actions github-actions bot removed the Type: RFC Request for Feedback label Apr 24, 2024
@layershifter
Copy link
Member Author

@levithomason @spmonahan @miroslavstastny @ling1726 @Hotell

The RFC have been updated based on the discussed happened in offline at 24th April.

  • Option A was rejected as it will cause breaking changes and will cause performance regressions.
  • Instead of prefixing (.fabdfx => .prefixfabdfx) we agreed to use prefix as a salt for hash (.fabdfx => .fvstsq).

@Hotell
Copy link
Contributor

Hotell commented May 15, 2024

The RFC have been updated based on the discussed happened in offline at 24th April.

  • Option A was rejected as it will cause breaking changes and will cause performance regressions.

To make things clear from semver POV, if we stop preprocessing our styles, that's not a Breaking Change - no public API surface will change, only thing that will change is that applications might get some performance hit (how big depends case by case, and it this should be clearly communicated to core partners - with intent to onboard them on application level AOT).

regarding the artificial export map additions: I don't think this is good way forward - reasoning provided inline.

things that need to be measured:

  • how big is perf hit with current workaround ( prefix of every style )
  • how big perf hit are we talking within core partners apps when disabling pre-processing within fluent (react-components)

After we gather proper data and perf hits are significant, we should help onboard core partners to adopt AOT approach within their build systems, as that's the main performance benefit of using our custom css-in-js solution, if folks don't use it they probably don't care about this metric.

After interested parties are onboarded, we should go with solution 1 = stop preprocessing react-components (and also probably archive/delete that babel plugin and docs about libraries pre-processing).

@Hotell Hotell added the Type: RFC Request for Feedback label May 15, 2024
@JustSlone
Copy link
Collaborator

The RFC have been updated based on the discussed happened in offline at 24th April.

  • Option A was rejected as it will cause breaking changes and will cause performance regressions.

To make things clear from semver POV, if we stop preprocessing our styles, that's not a Breaking Change - no public API surface will change, only thing that will change is that applications might get some performance hit (how big depends case by case, and it this should be clearly communicated to core partners - with intent to onboard them on application level AOT).

regarding the artificial export map additions: I don't think this is good way forward - reasoning provided inline.

things that need to be measured:

  • how big is perf hit with current workaround ( prefix of every style )
  • how big perf hit are we talking within core partners apps when disabling pre-processing within fluent (react-components)

After we gather proper data and perf hits are significant, we should help onboard core partners to adopt AOT approach within their build systems, as that's the main performance benefit of using our custom css-in-js solution, if folks don't use it they probably don't care about this metric.

After interested parties are onboarded, we should go with solution 1 = stop preprocessing react-components (and also probably archive/delete that babel plugin and docs about libraries pre-processing).

I had a similar initial intuition, if we could manage to do this in a 2-phase non-breaking change that would be ideal.

  1. Ship/communicate the right way to process styles, perhaps everyone's already doing this
  2. Make the change, communicate how to avoid the behavior

We do need to be careful that specificity changes don't become effective breaking changes. This should not break runtime behavior as Griffel would still operate at runtime

@github-actions github-actions bot removed the Type: RFC Request for Feedback label Jul 11, 2024
@layershifter
Copy link
Member Author

We had another offline round of discussions at 22 May.

Outcome:

  • We will go with Option A i.e. stop processing

To ensure that it won't break anybody/cause issues:

  • We will communicate the change in advance for MS products & onboard them to use AOT
  • We will update public docs & communicate it publicly
  • Once after that Option A will be executed

Copy link
Contributor

@Hotell Hotell left a comment

Choose a reason for hiding this comment

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

LGTM

@layershifter layershifter merged commit 9dd0252 into microsoft:master Jul 18, 2024
21 checks passed
@layershifter layershifter deleted the docs/rfc-stop-transforms branch July 18, 2024 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants