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

chore: run Babel plugins only during build #20894

Closed
wants to merge 6 commits into from

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented Dec 2, 2021

Pull request checklist

Description of changes

This PR excludes @fluentui/babel-make-styles from being executed during Storybook workflow. Our existing hack does not work, see #20886 (comment) for details.

All .babelrc files now use env = build thus they will be skipped during Storybook starts (we should not use Babel plugin, we should use Webpack loader instead, #20925). build is enabled directly in scripts/tasks/babel.ts so no changes in pipelines are required.

If we will use production it will collide with build-storybook script that forces production mode.

PR contains following changes:

  • updates generator
  • applies changes to all rc files
  • enables build mode in babel.ts
  • change files with type "none"
  • Webpack loader hack removal

Testing

  • git clean -fdX to clean all ignored files
  • yarn to install Node modules
  • yarn workspace @fluentui/react-image to start Storybook without issues 💪
  • yarn buildto @fluentui/react-image to ensure that artifacts still processed with Babel plugins

image

@fabricteam
Copy link
Collaborator

fabricteam commented Dec 2, 2021

📊 Bundle size report

Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-accordion
Accordion (including children components)
58.251 kB
18.464 kB
react-avatar
Avatar
46.563 kB
13.396 kB
react-badge
Badge
23.71 kB
6.967 kB
react-badge
CounterBadge
24.592 kB
7.278 kB
react-badge
PresenceBadge
22.248 kB
6.527 kB
react-button
Button
28.199 kB
8.093 kB
react-button
CompoundButton
33.56 kB
9.063 kB
react-button
MenuButton
30.282 kB
8.805 kB
react-button
SplitButton
36.507 kB
9.879 kB
react-button
ToggleButton
37.436 kB
8.687 kB
react-card
Card - All
50.701 kB
14.957 kB
react-card
Card
45.969 kB
13.741 kB
react-card
CardFooter
7.613 kB
3.23 kB
react-card
CardHeader
8.973 kB
3.682 kB
react-card
CardPreview
7.804 kB
3.359 kB
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
175.42 kB
49.569 kB
react-components
react-components: FluentProvider & webLightTheme
32.682 kB
10.634 kB
react-divider
Divider
14.95 kB
5.394 kB
react-image
Image
10.048 kB
3.942 kB
react-input
Input
19.605 kB
6.348 kB
react-label
Label
8.32 kB
3.476 kB
react-link
Link
11.041 kB
4.488 kB
react-make-styles
makeStaticStyles (runtime)
7.602 kB
3.325 kB
react-make-styles
makeStyles + mergeClasses (runtime)
22.006 kB
8.311 kB
react-make-styles
makeStyles + mergeClasses (build time)
2.689 kB
1.23 kB
react-menu
Menu (including children components)
105.926 kB
32.636 kB
react-menu
Menu (including selectable components)
108.318 kB
33 kB
react-popover
Popover
101.972 kB
30.98 kB
react-portal
Portal
6.621 kB
2.226 kB
react-positioning
usePopper
22.808 kB
7.935 kB
react-provider
FluentProvider
14.069 kB
5.252 kB
react-slider
RangedSlider
39.906 kB
11.504 kB
react-slider
Slider
33.295 kB
10.327 kB
react-switch
Switch
27.12 kB
8.561 kB
react-text
Text - Default
10.729 kB
4.214 kB
react-text
Text - Wrappers
14.031 kB
4.558 kB
react-theme
Teams: all themes
29.506 kB
6.512 kB
react-theme
Teams: Light theme
18.517 kB
5.237 kB
react-tooltip
Tooltip
44.948 kB
15.494 kB
react-utilities
SSRProvider
189 B
161 B
🤖 This report was generated against c6c745b664df52f5cbdd5a190cc08cae971216e0

@codesandbox-ci
Copy link

codesandbox-ci bot commented Dec 2, 2021

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

Sandbox Source
@fluentui/react 8 starter Configuration
@fluentui/react-components 9 starter Configuration

@size-auditor
Copy link

size-auditor bot commented Dec 2, 2021

Asset size changes

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

Baseline commit: c6c745b664df52f5cbdd5a190cc08cae971216e0 (build)

@layershifter layershifter marked this pull request as ready for review December 2, 2021 14:44
@fabricteam
Copy link
Collaborator

fabricteam commented Dec 2, 2021

Perf Analysis (@fluentui/react)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 1021 1067 5000
BaseButton mount 1056 1108 5000
Breadcrumb mount 2932 2792 1000
ButtonNext mount 655 658 5000
Checkbox mount 1693 1708 5000
CheckboxBase mount 1397 1494 5000
ChoiceGroup mount 5011 5021 5000
ComboBox mount 1116 1132 1000
CommandBar mount 10693 10490 1000
ContextualMenu mount 8840 8947 1000
DefaultButton mount 1243 1336 5000
DetailsRow mount 4052 4053 5000
DetailsRowFast mount 3937 3993 5000
DetailsRowNoStyles mount 3746 3887 5000
Dialog mount 2750 2709 1000
DocumentCardTitle mount 274 299 1000
Dropdown mount 3423 3396 5000
FluentProviderNext mount 1913 1915 5000
FluentProviderWithTheme mount 274 272 10
FluentProviderWithTheme virtual-rerender 207 204 10
FluentProviderWithTheme virtual-rerender-with-unmount 300 274 10
FocusTrapZone mount 1942 2001 5000
FocusZone mount 2009 1948 5000
IconButton mount 1915 1906 5000
Label mount 495 491 5000
Layer mount 3115 3105 5000
Link mount 611 588 5000
MakeStyles mount 2007 2006 50000
MenuButton mount 1638 1619 5000
MessageBar mount 2086 2147 5000
Nav mount 3416 3376 1000
OverflowSet mount 1257 1243 5000
Panel mount 2796 2571 1000
Persona mount 976 1001 1000
Pivot mount 1561 1571 1000
PrimaryButton mount 1615 1510 5000
Rating mount 8008 7995 5000
SearchBox mount 1485 1439 5000
Shimmer mount 2574 2642 5000
Slider mount 2074 2041 5000
SpinButton mount 5156 5121 5000
Spinner mount 522 574 5000
SplitButton mount 3351 3317 5000
Stack mount 616 646 5000
StackWithIntrinsicChildren mount 1789 1821 5000
StackWithTextChildren mount 4862 4710 5000
SwatchColorPicker mount 10574 10723 5000
TagPicker mount 2690 2781 5000
TeachingBubble mount 13435 13339 5000
Text mount 572 565 5000
TextField mount 1537 1493 5000
ThemeProvider mount 1339 1357 5000
ThemeProvider virtual-rerender 771 736 5000
ThemeProvider virtual-rerender-with-unmount 2028 2050 5000
Toggle mount 924 914 5000
buttonNative mount 245 246 5000

Perf Analysis (@fluentui/react-northstar)

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
PortalMinimalPerf.default 199 180 1.11:1
BoxMinimalPerf.default 374 340 1.1:1
ChatMinimalPerf.default 707 655 1.08:1
TableManyItemsPerf.default 2083 1923 1.08:1
AccordionMinimalPerf.default 175 164 1.07:1
FormMinimalPerf.default 437 407 1.07:1
ListMinimalPerf.default 565 528 1.07:1
ChatWithPopoverPerf.default 427 403 1.06:1
PopupMinimalPerf.default 642 605 1.06:1
TreeWith60ListItems.default 196 185 1.06:1
VideoMinimalPerf.default 703 665 1.06:1
ImageMinimalPerf.default 402 383 1.05:1
AttachmentSlotsPerf.default 1170 1130 1.04:1
AvatarMinimalPerf.default 227 218 1.04:1
CarouselMinimalPerf.default 517 499 1.04:1
HeaderMinimalPerf.default 374 359 1.04:1
LoaderMinimalPerf.default 729 700 1.04:1
DropdownManyItemsPerf.default 714 695 1.03:1
StatusMinimalPerf.default 722 702 1.03:1
TextMinimalPerf.default 366 356 1.03:1
TreeMinimalPerf.default 875 853 1.03:1
ButtonOverridesMissPerf.default 1721 1688 1.02:1
CardMinimalPerf.default 587 574 1.02:1
CheckboxMinimalPerf.default 2868 2816 1.02:1
DialogMinimalPerf.default 809 797 1.02:1
ItemLayoutMinimalPerf.default 1253 1230 1.02:1
LabelMinimalPerf.default 417 410 1.02:1
RosterPerf.default 1267 1238 1.02:1
RefMinimalPerf.default 254 250 1.02:1
TableMinimalPerf.default 430 422 1.02:1
TextAreaMinimalPerf.default 514 505 1.02:1
ButtonMinimalPerf.default 174 172 1.01:1
HeaderSlotsPerf.default 808 798 1.01:1
InputMinimalPerf.default 1358 1342 1.01:1
LayoutMinimalPerf.default 403 398 1.01:1
ListCommonPerf.default 695 688 1.01:1
MenuButtonMinimalPerf.default 1715 1690 1.01:1
IconMinimalPerf.default 653 645 1.01:1
AnimationMinimalPerf.default 456 458 1:1
AttachmentMinimalPerf.default 172 172 1:1
DatepickerMinimalPerf.default 6067 6079 1:1
DividerMinimalPerf.default 387 386 1:1
DropdownMinimalPerf.default 3161 3175 1:1
FlexMinimalPerf.default 308 308 1:1
MenuMinimalPerf.default 877 879 1:1
ProviderMinimalPerf.default 1193 1188 1:1
RadioGroupMinimalPerf.default 493 494 1:1
GridMinimalPerf.default 370 374 0.99:1
ProviderMergeThemesPerf.default 1787 1799 0.99:1
CustomToolbarPrototype.default 4327 4367 0.99:1
TooltipMinimalPerf.default 1090 1096 0.99:1
ChatDuplicateMessagesPerf.default 324 330 0.98:1
EmbedMinimalPerf.default 4259 4335 0.98:1
ListNestedPerf.default 593 603 0.98:1
SplitButtonMinimalPerf.default 4441 4536 0.98:1
ListWith60ListItems.default 680 700 0.97:1
ReactionMinimalPerf.default 384 395 0.97:1
SegmentMinimalPerf.default 361 373 0.97:1
SliderMinimalPerf.default 1717 1762 0.97:1
ButtonSlotsPerf.default 568 594 0.96:1
ToolbarMinimalPerf.default 1026 1067 0.96:1
SkeletonMinimalPerf.default 351 371 0.95:1
AlertMinimalPerf.default 286 316 0.91:1

@Hotell
Copy link
Contributor

Hotell commented Dec 2, 2021

why build ? FWIW it is not a standard environment variable. cant we use production ?

it would be nice to discuss this with vbuild team. in the past we discussed this problem in general and decided to not proceed with this approach yet. #18775

@Hotell
Copy link
Contributor

Hotell commented Dec 2, 2021

PR contains three atomic commits:

this is bit confusing, I see there are more than 3 commits, and those atomics are not valid. Can we remove this from the description to avoid confusion ? tip: If you wanna make changes and keep commits focused use --fixup

@ling1726
Copy link
Member

ling1726 commented Dec 2, 2021

why build ? FWIW it is not a standard environment variable. cant we use production ?

it would be nice to discuss this with vbuild team. in the past we discussed this problem in general and decided to not proceed with this approach yet. #18775
@Hotell

build-storybook runs with NODE_ENV=production which means that storybook build and e2e job in CI will break. Since building the storybooks does not build the babel-make-styles plugin.

@layershifter
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@varholak-peter varholak-peter requested a review from Hotell December 3, 2021 14:17
@Hotell
Copy link
Contributor

Hotell commented Dec 3, 2021

build-storybook runs with NODE_ENV=production which means that storybook build and e2e job in CI will break. Since building the storybooks does not build the babel-make-styles plugin.

@ling1726 thanks for refreshing my memory 🙌. In that case we should use indeed production as environmnent variable and for e2e and for react-components storybook build we should add yarn lage --to @flunetui/babel-make-styles as PRE step.

@ling1726
Copy link
Member

ling1726 commented Dec 6, 2021

build-storybook runs with NODE_ENV=production which means that storybook build and e2e job in CI will break. Since building the storybooks does not build the babel-make-styles plugin.

@ling1726 thanks for refreshing my memory 🙌. In that case we should use indeed production as environmnent variable and for e2e and for react-components storybook build we should add yarn lage --to @flunetui/babel-make-styles as PRE step.

@layershifter can you create an issue for this and assign to me ? so that it doens't block this fix

@layershifter
Copy link
Member Author

layershifter commented Dec 6, 2021

build-storybook runs with NODE_ENV=production which means that storybook build and e2e job in CI will break. Since building the storybooks does not build the babel-make-styles plugin.

@ling1726 thanks for refreshing my memory 🙌. In that case we should use indeed production as environmnent variable and for e2e and for react-components storybook build we should add yarn lage --to @flunetui/babel-make-styles as PRE step.

In reality it causes more interesting issue:

ERR! Module build failed (from ../../node_modules/babel-loader/lib/index.js):
ERR! TypeError: office-ui-fabric-react\packages\react-components\src\Concepts\Positioning\PositioningCoverTarget.stories.tsx: Cannot set property '0' of null
ERR!     at NodePath.replaceWithMultiple (office-ui-fabric-react\node_modules\@babel\traverse\lib\path\replacement.js:35:40)
ERR!     at Object.evaluatePathsInVM (office-ui-fabric-react\packages\babel-make-styles\lib\utils\evaluatePathsInVM.js:132:16)
ERR!     at Object.evaluatePaths (office-ui-fabric-react\packages\babel-make-styles\lib\utils\evaluatePaths.js:22:29)
ERR!     at PluginPass.exit (office-ui-fabric-react\packages\babel-make-styles\lib\plugin.js:404:41)
ERR!     at newFn (office-ui-fabric-react\node_modules\@babel\traverse\lib\visitors.js:171:21)
ERR!     at NodePath._call (office-ui-fabric-react\node_modules\@babel\traverse\lib\path\context.js:53:20)

That I am slowly debugging as it goes down to Babel internals a race condition (#20924).


The fact is that we should not run Babel plugins for Storybook, we should use Webpack's loader: otherwise we will get sooner or later issues with paths resolution.

Edit: issue created, update PR's description

@layershifter
Copy link
Member Author

layershifter commented Dec 6, 2021

build-storybook runs with NODE_ENV=production which means that storybook build and e2e job in CI will break. Since building the storybooks does not build the babel-make-styles plugin.

@ling1726 thanks for refreshing my memory 🙌. In that case we should use indeed production as environmnent variable and for e2e and for react-components storybook build we should add yarn lage --to @flunetui/babel-make-styles as PRE step.

@layershifter can you create an issue for this and assign to me ? so that it doens't block this fix

I created an issue to enable Webpack loader, #20925 👍

Copy link
Contributor

@behowell behowell left a comment

Choose a reason for hiding this comment

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

Signing off for any changes to the .babelrc.json files owned by cxe-red

@layershifter layershifter deleted the fix/buildless-env branch December 20, 2022 12:05
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.

cannot start Storybook on latest master in react-components
6 participants