RFC: Do not allow CSS shorthands in makeStyles() calls
#20573
Conversation
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: c4e248bbe45620352dc76e2b200f9163641c399d (build) |
📊 Bundle size reportUnchanged fixtures
|
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 1045 | 1061 | 5000 | |
| BaseButton | mount | 1039 | 1038 | 5000 | |
| Breadcrumb | mount | 2795 | 2765 | 1000 | |
| ButtonNext | mount | 576 | 558 | 5000 | |
| Checkbox | mount | 1695 | 1729 | 5000 | |
| CheckboxBase | mount | 1490 | 1464 | 5000 | |
| ChoiceGroup | mount | 5258 | 5228 | 5000 | |
| ComboBox | mount | 1041 | 1029 | 1000 | |
| CommandBar | mount | 10847 | 10850 | 1000 | |
| ContextualMenu | mount | 8917 | 8969 | 1000 | |
| DefaultButton | mount | 1273 | 1289 | 5000 | |
| DetailsRow | mount | 4109 | 4090 | 5000 | |
| DetailsRowFast | mount | 4166 | 4175 | 5000 | |
| DetailsRowNoStyles | mount | 3961 | 3981 | 5000 | |
| Dialog | mount | 2769 | 2801 | 1000 | |
| DocumentCardTitle | mount | 174 | 170 | 1000 | |
| Dropdown | mount | 3607 | 3570 | 5000 | |
| FluentProviderNext | mount | 4004 | 4065 | 5000 | |
| FluentProviderWithTheme | mount | 239 | 233 | 10 | |
| FluentProviderWithTheme | virtual-rerender | 105 | 99 | 10 | |
| FluentProviderWithTheme | virtual-rerender-with-unmount | 264 | 272 | 10 | |
| FocusTrapZone | mount | 1894 | 1943 | 5000 | |
| FocusZone | mount | 1952 | 1929 | 5000 | |
| IconButton | mount | 1964 | 1932 | 5000 | |
| Label | mount | 388 | 388 | 5000 | |
| Layer | mount | 3223 | 3368 | 5000 | |
| Link | mount | 528 | 527 | 5000 | |
| MakeStyles | mount | 1909 | 1874 | 50000 | |
| MenuButton | mount | 1664 | 1660 | 5000 | |
| MessageBar | mount | 2140 | 2150 | 5000 | |
| Nav | mount | 3691 | 3633 | 1000 | |
| OverflowSet | mount | 1188 | 1232 | 5000 | |
| Panel | mount | 2693 | 2607 | 1000 | |
| Persona | mount | 933 | 926 | 1000 | |
| Pivot | mount | 1589 | 1615 | 1000 | |
| PrimaryButton | mount | 1460 | 1417 | 5000 | |
| Rating | mount | 8809 | 8809 | 5000 | |
| SearchBox | mount | 1484 | 1512 | 5000 | |
| Shimmer | mount | 2842 | 2809 | 5000 | |
| Slider | mount | 2142 | 2157 | 5000 | |
| SpinButton | mount | 5473 | 5390 | 5000 | |
| Spinner | mount | 456 | 435 | 5000 | |
| SplitButton | mount | 3430 | 3406 | 5000 | |
| Stack | mount | 566 | 564 | 5000 | |
| StackWithIntrinsicChildren | mount | 1890 | 1931 | 5000 | |
| StackWithTextChildren | mount | 5353 | 5289 | 5000 | |
| SwatchColorPicker | mount | 11320 | 11337 | 5000 | |
| TagPicker | mount | 2857 | 2866 | 5000 | |
| TeachingBubble | mount | 14036 | 13941 | 5000 | |
| Text | mount | 477 | 483 | 5000 | |
| TextField | mount | 1566 | 1581 | 5000 | |
| ThemeProvider | mount | 1258 | 1290 | 5000 | |
| ThemeProvider | virtual-rerender | 644 | 648 | 5000 | |
| ThemeProvider | virtual-rerender-with-unmount | 2048 | 2058 | 5000 | |
| Toggle | mount | 901 | 905 | 5000 | |
| buttonNative | mount | 135 | 145 | 5000 |
Perf Analysis (@fluentui/react-northstar)
Perf tests with no regressions
| Scenario | Current PR Ticks | Baseline Ticks | Ratio |
|---|---|---|---|
| TreeWith60ListItems.default | 206 | 181 | 1.14:1 |
| TextMinimalPerf.default | 415 | 382 | 1.09:1 |
| ChatDuplicateMessagesPerf.default | 362 | 334 | 1.08:1 |
| PortalMinimalPerf.default | 199 | 186 | 1.07:1 |
| AnimationMinimalPerf.default | 475 | 449 | 1.06:1 |
| ButtonMinimalPerf.default | 206 | 194 | 1.06:1 |
| FlexMinimalPerf.default | 328 | 308 | 1.06:1 |
| ToolbarMinimalPerf.default | 1073 | 1017 | 1.06:1 |
| FormMinimalPerf.default | 498 | 475 | 1.05:1 |
| LoaderMinimalPerf.default | 759 | 722 | 1.05:1 |
| ReactionMinimalPerf.default | 438 | 416 | 1.05:1 |
| AvatarMinimalPerf.default | 233 | 223 | 1.04:1 |
| DividerMinimalPerf.default | 423 | 405 | 1.04:1 |
| ListCommonPerf.default | 742 | 713 | 1.04:1 |
| RefMinimalPerf.default | 259 | 249 | 1.04:1 |
| CarouselMinimalPerf.default | 548 | 533 | 1.03:1 |
| DatepickerMinimalPerf.default | 6082 | 5921 | 1.03:1 |
| ProviderMergeThemesPerf.default | 1802 | 1743 | 1.03:1 |
| SegmentMinimalPerf.default | 407 | 394 | 1.03:1 |
| TableManyItemsPerf.default | 2174 | 2119 | 1.03:1 |
| TooltipMinimalPerf.default | 1198 | 1160 | 1.03:1 |
| ButtonSlotsPerf.default | 633 | 618 | 1.02:1 |
| DialogMinimalPerf.default | 847 | 832 | 1.02:1 |
| DropdownManyItemsPerf.default | 786 | 769 | 1.02:1 |
| SkeletonMinimalPerf.default | 398 | 392 | 1.02:1 |
| SplitButtonMinimalPerf.default | 4759 | 4669 | 1.02:1 |
| AttachmentMinimalPerf.default | 183 | 182 | 1.01:1 |
| BoxMinimalPerf.default | 391 | 389 | 1.01:1 |
| ButtonOverridesMissPerf.default | 1891 | 1878 | 1.01:1 |
| ChatMinimalPerf.default | 756 | 748 | 1.01:1 |
| ChatWithPopoverPerf.default | 433 | 427 | 1.01:1 |
| DropdownMinimalPerf.default | 3255 | 3230 | 1.01:1 |
| EmbedMinimalPerf.default | 4617 | 4558 | 1.01:1 |
| GridMinimalPerf.default | 382 | 378 | 1.01:1 |
| ItemLayoutMinimalPerf.default | 1381 | 1367 | 1.01:1 |
| LayoutMinimalPerf.default | 427 | 421 | 1.01:1 |
| MenuButtonMinimalPerf.default | 1805 | 1781 | 1.01:1 |
| SliderMinimalPerf.default | 1819 | 1806 | 1.01:1 |
| StatusMinimalPerf.default | 747 | 742 | 1.01:1 |
| CheckboxMinimalPerf.default | 2928 | 2916 | 1:1 |
| HeaderMinimalPerf.default | 406 | 404 | 1:1 |
| HeaderSlotsPerf.default | 852 | 853 | 1:1 |
| RadioGroupMinimalPerf.default | 512 | 513 | 1:1 |
| TableMinimalPerf.default | 439 | 440 | 1:1 |
| TextAreaMinimalPerf.default | 562 | 560 | 1:1 |
| CustomToolbarPrototype.default | 4310 | 4314 | 1:1 |
| TreeMinimalPerf.default | 887 | 883 | 1:1 |
| AttachmentSlotsPerf.default | 1151 | 1166 | 0.99:1 |
| ImageMinimalPerf.default | 425 | 429 | 0.99:1 |
| ListNestedPerf.default | 608 | 614 | 0.99:1 |
| MenuMinimalPerf.default | 941 | 948 | 0.99:1 |
| PopupMinimalPerf.default | 627 | 634 | 0.99:1 |
| IconMinimalPerf.default | 677 | 686 | 0.99:1 |
| AlertMinimalPerf.default | 313 | 320 | 0.98:1 |
| InputMinimalPerf.default | 1423 | 1446 | 0.98:1 |
| LabelMinimalPerf.default | 440 | 448 | 0.98:1 |
| ListMinimalPerf.default | 576 | 587 | 0.98:1 |
| ListWith60ListItems.default | 707 | 721 | 0.98:1 |
| CardMinimalPerf.default | 639 | 657 | 0.97:1 |
| RosterPerf.default | 1334 | 1375 | 0.97:1 |
| ProviderMinimalPerf.default | 1190 | 1228 | 0.97:1 |
| AccordionMinimalPerf.default | 176 | 184 | 0.96:1 |
| VideoMinimalPerf.default | 678 | 718 | 0.94:1 |
|
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 aac2298:
|
miroslavstastny
assigned miroslavstastny and layershifter and unassigned miroslavstastny
| makeStyles({ | ||
| rootA: { paddingLeft: '4px', paddingRight: '4px', paddingBottom: '4px', paddingTop: '4px' }, | ||
| rootB: { | ||
| ...macros.padding('4px'), |
There was a problem hiding this comment.
For ex-C++ developers, the word macros has a lot of history to it :) How about calling it shorthand(), fui(), makeCss()?
There was a problem hiding this comment.
Yeah, it also has overlap with "Babel macros". I will rename it to shorthands.*, that makes more sense to me 👍
| - 👍 No CSS shorthands => no problems | ||
| - 👍 No new syntax | ||
| - 👎 Non obvious TypeScript errors in some cases | ||
|
|
There was a problem hiding this comment.
I would add a con that we have to explain this to users of makeStyles in our documentation. It causes Fluent to seem a little less aligned with web standards since people would expect to use shorthand with raw CSS and with most frameworks.
There was a problem hiding this comment.
Yeah, true, added 👍
There was a problem hiding this comment.
I'm afraid I don't have the full context so please take this with a grain of salt, but it doesn't make sense to me that complicating DX should be justified with adding documentation. As a user, I would expect a styling library to allow me to write CSS like I normally would.
Could we maybe contribute to inline-style-expand-shorthand to add the support we need instead or is this what you meant in the issue that it's not feasible in terms of complexity?
There was a problem hiding this comment.
@andrefcdias I agree, but during offline discussion we agreed to make something always predictable instead of sometimes working. Currently it's sometimes working.
The problem is covered in the summary section of this RFC and in the original issue #19402. There are different cases: in some it's about complexity, but there is the main case when it's about possibility at all:
{ padding: 'var(--some-var)' }👆 CSS variable can be anything (🗃️ CodeSandbox), it means that it's unsafe to expand it at all.
To summarize:
- CSS variables are not expandable at all during build time (we don't have CSS variables in Node environment even if we would like to resolve and expand)
- CSS can be resolved in runtime via calls of
getPropertyValue()(🗃️ CodeSandbox), but will be too expensive as it should be done for every component and every usage ofthemeor CSS variables- Note 1:
getPropertyValue()should be called after render as during we may not have DOM at all (👋 double rendering) - Note 2: we need to handle updates (How at all?)
- Note 1:
The same problem is true for any CSS-in-JS engine that uses Atomic CSS (Compiled, Stylex, Fela, etc.).
There was a problem hiding this comment.
Thanks for clarifying! Just wanted to make sure we covered all our options.
|
|
||
| - 👍 No CSS shorthands => no problems | ||
| - 👍 No new syntax | ||
| - 👎 Non obvious TypeScript errors in some cases |
There was a problem hiding this comment.
Consider if a custom es-lint rule might help to convert shorthand to the macro function or provide better error information.
| - `borderRadius` | ||
| - `borderWidth` | ||
| - `margin` | ||
| - `padding` |
There was a problem hiding this comment.
Consider adding background.
Lower priority, it would be a bonus to have a few for common shorthand in flexbox and grid.
There was a problem hiding this comment.
Yeah it's one of the first things that I was thinking, but it's not obvious unfortunately as background value could also image:
// ✅ Easy-peasy
shorthands.background("green");
// ➡️ { backgroundColor: 'green' }
// 💥 Oops
shorthands.background('url("lizard.png")');
// ➡️ { backgroundColor: 'url("lizard.png")' }https://developer.mozilla.org/en-US/docs/Web/CSS/background
Any ideas?
Co-authored-by: Miroslav Stastny <mistastn@microsoft.com>
So I am merging it. If there will be any ideas how we could solve in a better way - we can always re-consider implementation and the decision. |
…0573) * RFC: Do not allow CSS shorthands in `makeStyles()` calls * rename "macros" to "shorthands" * add one more con * add note about ESLint rules * Update rfcs/convergence/no-css-shorthands-in-make-styles.md Co-authored-by: Miroslav Stastny <mistastn@microsoft.com> Co-authored-by: Miroslav Stastny <mistastn@microsoft.com>
Description of changes
This RFC proposed to forbid usage of CSS shorthands in
makeStyles()calls. Check also #19402 for context.📖 Rendered preview