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

Migrate storybook to 6.2.9 #18693

Merged
merged 52 commits into from
Jul 22, 2021

Conversation

TristanWatanabe
Copy link
Member

@TristanWatanabe TristanWatanabe commented Jun 24, 2021

Pull request checklist

Description of changes

@TristanWatanabe
Copy link
Member Author

The babel warnings below is a known issue and was fixed in 6.3.x
image

@size-auditor
Copy link

size-auditor bot commented Jun 24, 2021

Asset size changes

Project Bundle Baseline Size New Size Difference
office-ui-fabric-react fluentui-react-northstar-Tooltip 119.508 kB 119.51 kB ExceedsBaseline     2 bytes
office-ui-fabric-react fluentui-react-northstar-Toolbar 186.905 kB 186.907 kB ExceedsBaseline     2 bytes
office-ui-fabric-react fluentui-react-northstar-Menu 140.928 kB 140.93 kB ExceedsBaseline     2 bytes
office-ui-fabric-react fluentui-react-northstar-MenuButton 174.844 kB 174.846 kB ExceedsBaseline     2 bytes
office-ui-fabric-react fluentui-react-northstar-Popup 144.915 kB 144.917 kB ExceedsBaseline     2 bytes
office-ui-fabric-react fluentui-react-northstar-SplitButton 190.05 kB 190.052 kB ExceedsBaseline     2 bytes
office-ui-fabric-react fluentui-react-northstar-Chat 164.426 kB 164.428 kB ExceedsBaseline     2 bytes
office-ui-fabric-react fluentui-react-northstar-Dropdown 209.746 kB 209.748 kB ExceedsBaseline     2 bytes
office-ui-fabric-react fluentui-react-northstar-Accordion 97.282 kB 97.283 kB ExceedsBaseline     1 bytes
office-ui-fabric-react fluentui-react-northstar-Reaction 88.492 kB 88.493 kB ExceedsBaseline     1 bytes
office-ui-fabric-react fluentui-react-northstar-Layout 86.453 kB 86.454 kB ExceedsBaseline     1 bytes
office-ui-fabric-react fluentui-react-northstar-Loader 89.773 kB 89.774 kB ExceedsBaseline     1 bytes
office-ui-fabric-react fluentui-react-northstar-Pill 94.977 kB 94.978 kB ExceedsBaseline     1 bytes
office-ui-fabric-react fluentui-react-northstar-RadioGroup 95.034 kB 95.035 kB ExceedsBaseline     1 bytes
office-ui-fabric-react fluentui-react-northstar-Slider 96.011 kB 96.012 kB ExceedsBaseline     1 bytes
office-ui-fabric-react fluentui-react-northstar-Segment 87.32 kB 87.321 kB ExceedsBaseline     1 bytes
office-ui-fabric-react fluentui-react-northstar-Skeleton 88.814 kB 88.815 kB ExceedsBaseline     1 bytes
office-ui-fabric-react fluentui-react-northstar-ItemLayout 89.459 kB 89.46 kB ExceedsBaseline     1 bytes
office-ui-fabric-react fluentui-react-northstar-Status 87.508 kB 87.509 kB ExceedsBaseline     1 bytes
office-ui-fabric-react fluentui-react-northstar-Table 92.4 kB 92.401 kB ExceedsBaseline     1 bytes
office-ui-fabric-react fluentui-react-northstar-Text 85.088 kB 85.089 kB ExceedsBaseline     1 bytes
office-ui-fabric-react fluentui-react-northstar-TextArea 85.216 kB 85.217 kB ExceedsBaseline     1 bytes
office-ui-fabric-react fluentui-react-northstar-Tree 101.533 kB 101.534 kB ExceedsBaseline     1 bytes
office-ui-fabric-react fluentui-react-northstar-Video 86.441 kB 86.442 kB ExceedsBaseline     1 bytes
office-ui-fabric-react fluentui-react-northstar-Label 89.117 kB 89.118 kB ExceedsBaseline     1 bytes
office-ui-fabric-react fluentui-react-northstar-List 100.623 kB 100.624 kB ExceedsBaseline     1 bytes
office-ui-fabric-react fluentui-react-northstar-Input 99.442 kB 99.443 kB ExceedsBaseline     1 bytes
office-ui-fabric-react fluentui-react-northstar-Dialog 123.818 kB 123.819 kB ExceedsBaseline     1 bytes
office-ui-fabric-react fluentui-react-northstar-Alert 99.217 kB 99.218 kB ExceedsBaseline     1 bytes
office-ui-fabric-react fluentui-react-northstar-Attachment 98.419 kB 98.42 kB ExceedsBaseline     1 bytes
office-ui-fabric-react fluentui-react-northstar-Avatar 90.616 kB 90.617 kB ExceedsBaseline     1 bytes
office-ui-fabric-react fluentui-react-northstar-Box 86.333 kB 86.334 kB ExceedsBaseline     1 bytes
office-ui-fabric-react fluentui-react-northstar-Breadcrumb 91.248 kB 91.249 kB ExceedsBaseline     1 bytes
office-ui-fabric-react fluentui-react-northstar-Button 94.557 kB 94.558 kB ExceedsBaseline     1 bytes
office-ui-fabric-react fluentui-react-northstar-Card 94.057 kB 94.058 kB ExceedsBaseline     1 bytes
office-ui-fabric-react fluentui-react-northstar-Image 84.491 kB 84.492 kB ExceedsBaseline     1 bytes
office-ui-fabric-react fluentui-react-northstar-Checkbox 91.416 kB 91.417 kB ExceedsBaseline     1 bytes
office-ui-fabric-react fluentui-react-northstar-Carousel 115.475 kB 115.476 kB ExceedsBaseline     1 bytes
office-ui-fabric-react fluentui-react-northstar-Grid 81.311 kB 81.312 kB ExceedsBaseline     1 bytes
office-ui-fabric-react fluentui-react-northstar-Divider 87.755 kB 87.756 kB ExceedsBaseline     1 bytes
office-ui-fabric-react fluentui-react-northstar-Embed 92.991 kB 92.992 kB ExceedsBaseline     1 bytes
office-ui-fabric-react fluentui-react-northstar-Form 103.815 kB 103.816 kB ExceedsBaseline     1 bytes
office-ui-fabric-react fluentui-react-northstar-Header 85.763 kB 85.764 kB ExceedsBaseline     1 bytes
office-ui-fabric-react fluentui-react-northstar-Datepicker 201.183 kB 199.098 kB BelowBaseline     -2.085 kB

ExceedsTolerance Over Tolerance (1024 B) ExceedsBaseline Over Baseline BelowBaseline Below Baseline New New Deleted  Removed 1 kB = 1000 B

Baseline commit: 465a840933b39e5a54c0492180543806542b24b7 (build)

@fabricteam
Copy link
Collaborator

fabricteam commented Jun 24, 2021

Perf Analysis (@fluentui/react)

Scenario Render type Master Ticks PR Ticks Iterations Status
buttonNative mount 110 120 5000 Possible regression
All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 802 845 5000
BaseButton mount 894 868 5000
Breadcrumb mount 2628 2581 1000
ButtonNext mount 524 518 5000
Checkbox mount 1530 1480 5000
CheckboxBase mount 1269 1279 5000
ChoiceGroup mount 4712 4673 5000
ComboBox mount 961 980 1000
CommandBar mount 10073 10222 1000
ContextualMenu mount 6222 6286 1000
DefaultButton mount 1110 1114 5000
DetailsRow mount 3663 3660 5000
DetailsRowFast mount 3670 3700 5000
DetailsRowNoStyles mount 3530 3493 5000
Dialog mount 2175 2177 1000
DocumentCardTitle mount 137 145 1000
Dropdown mount 3216 3227 5000
FluentProviderNext mount 7184 7357 5000
FocusTrapZone mount 1777 1795 5000
FocusZone mount 1864 1817 5000
IconButton mount 1721 1747 5000
Label mount 335 343 5000
Layer mount 1810 1776 5000
Link mount 454 464 5000
MakeStyles mount 1783 1812 50000
MenuButton mount 1428 1399 5000
MessageBar mount 1997 2003 5000
Nav mount 3243 3210 1000
OverflowSet mount 1053 1021 5000
Panel mount 2102 2104 1000
Persona mount 798 790 1000
Pivot mount 1399 1411 1000
PrimaryButton mount 1294 1294 5000
Rating mount 7489 7704 5000
SearchBox mount 1323 1308 5000
Shimmer mount 2546 2505 5000
Slider mount 1926 1976 5000
SpinButton mount 4877 4945 5000
Spinner mount 396 423 5000
SplitButton mount 3068 3124 5000
Stack mount 481 494 5000
StackWithIntrinsicChildren mount 1551 1542 5000
StackWithTextChildren mount 4500 4489 5000
SwatchColorPicker mount 10148 10241 5000
Tabs mount 1410 1392 1000
TagPicker mount 2421 2415 5000
TeachingBubble mount 11886 11823 5000
Text mount 416 408 5000
TextField mount 1371 1360 5000
ThemeProvider mount 1172 1179 5000
ThemeProvider virtual-rerender 584 596 5000
Toggle mount 822 815 5000
buttonNative mount 110 120 5000 Possible regression

Perf Analysis (@fluentui/react-northstar)

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
AccordionMinimalPerf.default 162 138 1.17:1
PortalMinimalPerf.default 187 174 1.07:1
FlexMinimalPerf.default 285 268 1.06:1
BoxMinimalPerf.default 353 335 1.05:1
ListWith60ListItems.default 649 621 1.05:1
RefMinimalPerf.default 246 235 1.05:1
PopupMinimalPerf.default 602 580 1.04:1
SkeletonMinimalPerf.default 348 335 1.04:1
ButtonSlotsPerf.default 543 528 1.03:1
GridMinimalPerf.default 340 331 1.03:1
ListMinimalPerf.default 506 492 1.03:1
SegmentMinimalPerf.default 349 339 1.03:1
StatusMinimalPerf.default 667 649 1.03:1
TableMinimalPerf.default 405 394 1.03:1
AlertMinimalPerf.default 273 268 1.02:1
AvatarMinimalPerf.default 196 192 1.02:1
CarouselMinimalPerf.default 448 440 1.02:1
CheckboxMinimalPerf.default 2739 2681 1.02:1
DropdownMinimalPerf.default 3109 3063 1.02:1
FormMinimalPerf.default 395 386 1.02:1
AttachmentMinimalPerf.default 159 157 1.01:1
ButtonMinimalPerf.default 165 164 1.01:1
ButtonOverridesMissPerf.default 1705 1690 1.01:1
CardMinimalPerf.default 544 539 1.01:1
ChatDuplicateMessagesPerf.default 296 294 1.01:1
LayoutMinimalPerf.default 368 363 1.01:1
ListCommonPerf.default 629 625 1.01:1
SplitButtonMinimalPerf.default 3778 3737 1.01:1
TextMinimalPerf.default 344 339 1.01:1
ToolbarMinimalPerf.default 936 931 1.01:1
TooltipMinimalPerf.default 1005 994 1.01:1
AttachmentSlotsPerf.default 1064 1060 1:1
ChatWithPopoverPerf.default 363 363 1:1
DatepickerMinimalPerf.default 5347 5330 1:1
EmbedMinimalPerf.default 4061 4051 1:1
HeaderMinimalPerf.default 346 346 1:1
LoaderMinimalPerf.default 684 684 1:1
MenuButtonMinimalPerf.default 1619 1622 1:1
ProviderMergeThemesPerf.default 1675 1672 1:1
SliderMinimalPerf.default 1580 1580 1:1
TableManyItemsPerf.default 1847 1851 1:1
CustomToolbarPrototype.default 3896 3899 1:1
DialogMinimalPerf.default 727 735 0.99:1
DividerMinimalPerf.default 343 347 0.99:1
HeaderSlotsPerf.default 734 744 0.99:1
ItemLayoutMinimalPerf.default 1180 1188 0.99:1
ProviderMinimalPerf.default 1004 1014 0.99:1
TextAreaMinimalPerf.default 479 485 0.99:1
TreeMinimalPerf.default 795 800 0.99:1
TreeWith60ListItems.default 169 170 0.99:1
DropdownManyItemsPerf.default 666 681 0.98:1
InputMinimalPerf.default 1246 1268 0.98:1
LabelMinimalPerf.default 373 379 0.98:1
ListNestedPerf.default 532 545 0.98:1
MenuMinimalPerf.default 814 829 0.98:1
IconMinimalPerf.default 599 614 0.98:1
RadioGroupMinimalPerf.default 432 445 0.97:1
VideoMinimalPerf.default 603 621 0.97:1
AnimationMinimalPerf.default 392 408 0.96:1
ChatMinimalPerf.default 634 659 0.96:1
ReactionMinimalPerf.default 360 380 0.95:1
ImageMinimalPerf.default 343 363 0.94:1
RosterPerf.default 1078 1181 0.91:1

@TristanWatanabe
Copy link
Member Author

TristanWatanabe commented Jun 24, 2021

Update: No longer an issue - updating the imports (as seen in this commit) fixes this


I can resolve the errors by adding /* @ts-ignore */ above each line but that would require updating each story files in apps/vr-tests. What do you think @ecraig12345 @Hotell ?

  .addDecorator(story => (
    <Screener steps={new Screener.Steps().snapshot('normal', { cropTo: '.testWrapper' }).end()}>
      <div className="testWrapper" style={{ width: '300px' }}>
        {story()}
      </div>
    </Screener>
  ))
  .addDecorator(FluentProviderDecorator)
  /* @ts-ignore */
  .addStory('size', () => (
    <Accordion index={[0, 1, 2, 3]}>
      <AccordionItem>

This error is introduced when upgrading storybook/react to 6.2.0-alpha.7 in apps/vr-tests. Changelog for that release:

image


apps/vr-tests now failing:
image

@TristanWatanabe
Copy link
Member Author

TristanWatanabe commented Jun 25, 2021

Update: tried storybook version 6.3.0-alpha.21 and this is no longer an issue there. Seems to be fixed by this PR


Running into a regression where codeblocks are not displayed properly for .mdx files. The last time this worked with our codebase was when I put addon-essentials at version 6.1.0-alpha.29. I ran a create-react app with storybook at version 6.2.9 and this wasn't an issue there so this might be caused by a conflict our own configs.

6.2.9
image

Current:
image

package.json Outdated
@@ -108,6 +109,7 @@
"@types/webpack-env": "1.16.0",
"@types/yargs": "13.0.11",
"ajv": "8.4.0",
"autoprefixer": "^9.7.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

please use fixed versions in root devDependencies

scripts/package.json Outdated Show resolved Hide resolved
postcss.config.js Outdated Show resolved Hide resolved
scripts/tasks/storybook.ts Show resolved Hide resolved
@@ -5,6 +5,10 @@ const config = createConfig({
setupFiles: [path.resolve(path.join(__dirname, 'config', 'tests.js'))],

snapshotSerializers: [resolveMergeStylesSerializer()],

moduleNameMapper: {
'@storybook/addon-docs/blocks$': '@storybook/addon-docs/dist/cjs/blocks',
Copy link
Contributor

Choose a reason for hiding this comment

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

q: is this a know limitation (to explicitly provide CJS storybooks output) ? I mean we don't run tests on stories do we?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure about the limitation but this is a known issue (see here) that's supposed to have been fixed in 6.3.x. Tests were written for the utils.tsx file in react-examples/react-components/Migrations (see file here) which caused the initial failure.

apps/vr-tests/src/utilities/index.ts Show resolved Hide resolved
@@ -23,6 +23,14 @@ module.exports = /** @type {Pick<StorybookConfig,'addons' |'stories' |'webpackFi
'@storybook/addon-a11y',
'@storybook/addon-knobs/preset',
'storybook-addon-performance',
{
name: '@storybook/addon-postcss',
Copy link
Contributor

Choose a reason for hiding this comment

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

q: why is this needed ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since we require postcss 8.2.4, this needed to be added per document snippet below:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm I don't think we need postcss for vNext (as styling is processed by make-styles - so vendor prefix etc are being properly added - am I right @layershifter ?)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it's correct, we don't need PostCSS for vNext components

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it - I will remove addon-postcss from the storybook addons and also remove the postcss.config.js file i created.

@TristanWatanabe TristanWatanabe requested a review from a team as a code owner July 19, 2021 15:29
@TristanWatanabe TristanWatanabe force-pushed the migrate-storybook branch 2 times, most recently from e60aba3 to 0a03584 Compare July 19, 2021 15:55
@@ -21,7 +20,6 @@ const storyOrder = [
'Migrations/Flex/Overview',
];

addDecorator(withInfo);
Copy link
Contributor

Choose a reason for hiding this comment

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

withInfo was replaced by some addon ?

Copy link
Member Author

Choose a reason for hiding this comment

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

As of now, i haven't added anything to replace it yet. Since addon-info is deprecated, it was not playing nice with the new storybook webpack5 builder so Elizabeth and I decided to remove it for now (see here)

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good, vNext uses storybook Docs anyways with typescript doc generation

Copy link
Contributor

@Hotell Hotell Jul 20, 2021

Choose a reason for hiding this comment

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

with that we can also remove our manual typings (can be done in separate PR)
2021-07-20 at 19 24

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll create a separate issue for this!

@TristanWatanabe
Copy link
Member Author

TristanWatanabe commented Jul 19, 2021

Update: No longer an issue; I ran yarn start-storybook --no-manager-cache as this comment suggested and that did the trick!


After merging with latest master; now running into an issue below when running yarn start on react-menu. This only happens with react-menu as other converged packages run fine. react-menu stories also load just fine when yarn start is ran for react-components.

image

@Hotell
Copy link
Contributor

Hotell commented Jul 20, 2021

there are 3 VTest regression - svg icons stories are ordered in different way (weird) - probably some underlying mechanism change of how 6.2. renders stories that use deprecated storiesOf apis ?

if folks are ok with that (@ecraig12345 @ling1726 ), lets approve those and ship this beauty! 😍

@ecraig12345
Copy link
Member

there are 3 VTest regression - svg icons stories are ordered in different way (weird) - probably some underlying mechanism change of how 6.2. renders stories that use deprecated storiesOf apis ?

if folks are ok with that, lets approve those and ship this beauty! 😍

That's really strange, it looks like previously they weren't fully alphabetized and now they are? But since all the icons are still there it should be fine (as long as it stays stable), so I accepted the changes.

.storybook/main.js Outdated Show resolved Hide resolved
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.

left some comments, but not blocking. Great work @TristanWatanabe 🙌

@msft-fluent-ui-bot
Copy link
Collaborator

Hello @TristanWatanabe!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msft-fluent-ui-bot) and give me an instruction to get started! Learn more here.

@msft-fluent-ui-bot msft-fluent-ui-bot merged commit 5fcdb72 into microsoft:master Jul 22, 2021
@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

PeterDraex pushed a commit to PeterDraex/fluentui that referenced this pull request Aug 6, 2021
#### Pull request checklist

- [x] Addresses an existing issue: Fixes microsoft#18142
- [x] Include a change request file using `$ yarn change`

#### Description of changes
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.

chore: migrate to latest storybook 6.2.x
8 participants