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

react-make-styles: Migrate to new DX. #18695

Merged

Conversation

TristanWatanabe
Copy link
Member

Pull request checklist

Description of changes

  • migrates the react-make-styles package to new DX using nx workspace-generator migrate-converged-pkg

@Hotell
Copy link
Contributor

Hotell commented Jun 25, 2021

CI won't pass as we run into corner case 🔥 hurray! :D ... kidding aside. Why is that?

  • because these collocated stories use @fluentui/react-provider and this package is not defined as a dependency/devDependency , lage is unable to process the build order properly thus we are getting tsc error as that package build was not executed yet

    • Note: you cannot specify it as dependency. If you specify it as devDep you'll get circular error from lage
      2021-06-25 at 13 31
  • we have plans to mitigate all of this with environment TSConfigs

  • also we are currently shipping those stories to npm as we compile everything. To mitigate this we need to pass --exclude **/*.stories.tsx when executing just tsc, but unfortunately, that's not possible (just limitation).

Temporary fix
To get this in, only quick fix that comes up to my mind is to @ts-ignore those 2 imports within stories

// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore - add link to this comment please  (see https://github.com/microsoft/fluentui/pull/18695)
import { FluentProvider } from '@fluentui/react-provider';

@size-auditor
Copy link

size-auditor bot commented Jun 26, 2021

Asset size changes

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

Baseline commit: a619fd6fb5f3895fa297dbcaa55b03548f6a6398 (build)

@fabricteam
Copy link
Collaborator

fabricteam commented Jun 26, 2021

Perf Analysis (@fluentui/react)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 842 876 5000
BaseButton mount 980 971 5000
Breadcrumb mount 2720 2691 1000
ButtonNext mount 543 540 5000
Checkbox mount 1626 1692 5000
CheckboxBase mount 1470 1395 5000
ChoiceGroup mount 5260 5192 5000
ComboBox mount 1009 1032 1000
CommandBar mount 10684 10545 1000
ContextualMenu mount 6604 6866 1000
DefaultButton mount 1236 1242 5000
DetailsRow mount 3924 3966 5000
DetailsRowFast mount 3923 3958 5000
DetailsRowNoStyles mount 3700 3770 5000
Dialog mount 2270 2262 1000
DocumentCardTitle mount 139 142 1000
Dropdown mount 3449 3451 5000
FocusTrapZone mount 1897 1844 5000
FocusZone mount 1907 1850 5000
IconButton mount 1927 1957 5000
Label mount 348 349 5000
Layer mount 1912 1916 5000
Link mount 503 487 5000
MakeStyles mount 1795 1801 50000
MenuButton mount 1583 1579 5000
MessageBar mount 2054 2081 5000
Nav mount 3502 3557 1000
OverflowSet mount 1121 1103 5000
Panel mount 2252 2221 1000
Persona mount 896 931 1000
Pivot mount 1562 1542 1000
PrimaryButton mount 1452 1454 5000
Rating mount 8789 8821 5000
SearchBox mount 1509 1536 5000
Shimmer mount 2857 3007 5000
Slider mount 2246 2214 5000
SpinButton mount 5546 5611 5000
Spinner mount 468 462 5000
SplitButton mount 3501 3489 5000
Stack mount 557 577 5000
StackWithIntrinsicChildren mount 1722 1727 5000
StackWithTextChildren mount 5263 5177 5000
SwatchColorPicker mount 11340 11549 5000
Tabs mount 1498 1518 1000
TagPicker mount 2642 2711 5000
TeachingBubble mount 12311 12561 5000
Text mount 465 468 5000
TextField mount 1486 1495 5000
ThemeProvider mount 1225 1245 5000
ThemeProvider virtual-rerender 598 607 5000
ThemeProviderNext mount 6922 6939 5000
Toggle mount 849 845 5000
buttonNative mount 115 112 5000

Perf Analysis (@fluentui/react-northstar)

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
ButtonMinimalPerf.default 209 172 1.22:1
AccordionMinimalPerf.default 188 170 1.11:1
AttachmentMinimalPerf.default 188 170 1.11:1
PortalMinimalPerf.default 199 180 1.11:1
TextAreaMinimalPerf.default 613 553 1.11:1
ChatDuplicateMessagesPerf.default 330 302 1.09:1
DropdownManyItemsPerf.default 796 740 1.08:1
ReactionMinimalPerf.default 453 425 1.07:1
ChatMinimalPerf.default 737 696 1.06:1
BoxMinimalPerf.default 407 389 1.05:1
SegmentMinimalPerf.default 389 372 1.05:1
TableMinimalPerf.default 476 455 1.05:1
TreeMinimalPerf.default 915 868 1.05:1
AlertMinimalPerf.default 315 302 1.04:1
ButtonSlotsPerf.default 620 595 1.04:1
LayoutMinimalPerf.default 428 413 1.04:1
ListMinimalPerf.default 578 554 1.04:1
LoaderMinimalPerf.default 792 758 1.04:1
StatusMinimalPerf.default 787 756 1.04:1
TooltipMinimalPerf.default 1116 1074 1.04:1
CardMinimalPerf.default 636 615 1.03:1
RefMinimalPerf.default 245 238 1.03:1
DatepickerMinimalPerf.default 5800 5682 1.02:1
EmbedMinimalPerf.default 4415 4348 1.02:1
FlexMinimalPerf.default 315 310 1.02:1
FormMinimalPerf.default 461 453 1.02:1
GridMinimalPerf.default 382 374 1.02:1
ImageMinimalPerf.default 432 424 1.02:1
InputMinimalPerf.default 1370 1338 1.02:1
ListCommonPerf.default 703 689 1.02:1
MenuMinimalPerf.default 930 911 1.02:1
RadioGroupMinimalPerf.default 512 500 1.02:1
TableManyItemsPerf.default 2221 2179 1.02:1
VideoMinimalPerf.default 681 670 1.02:1
AnimationMinimalPerf.default 455 450 1.01:1
AttachmentSlotsPerf.default 1195 1181 1.01:1
ButtonOverridesMissPerf.default 1795 1778 1.01:1
CheckboxMinimalPerf.default 2922 2880 1.01:1
DividerMinimalPerf.default 397 393 1.01:1
HeaderSlotsPerf.default 871 866 1.01:1
ItemLayoutMinimalPerf.default 1402 1383 1.01:1
SliderMinimalPerf.default 1662 1645 1.01:1
CustomToolbarPrototype.default 4131 4090 1.01:1
ToolbarMinimalPerf.default 1070 1062 1.01:1
ChatWithPopoverPerf.default 383 383 1:1
DialogMinimalPerf.default 821 824 1:1
DropdownMinimalPerf.default 3181 3189 1:1
HeaderMinimalPerf.default 414 412 1:1
MenuButtonMinimalPerf.default 1756 1754 1:1
PopupMinimalPerf.default 629 626 1:1
ProviderMinimalPerf.default 1027 1022 1:1
TreeWith60ListItems.default 181 181 1:1
AvatarMinimalPerf.default 210 212 0.99:1
ListNestedPerf.default 630 634 0.99:1
RosterPerf.default 1293 1303 0.99:1
TextMinimalPerf.default 395 401 0.99:1
LabelMinimalPerf.default 436 443 0.98:1
ListWith60ListItems.default 711 725 0.98:1
ProviderMergeThemesPerf.default 1694 1737 0.98:1
IconMinimalPerf.default 678 690 0.98:1
SkeletonMinimalPerf.default 388 401 0.97:1
SplitButtonMinimalPerf.default 4029 4139 0.97:1
CarouselMinimalPerf.default 489 513 0.95:1

@Hotell
Copy link
Contributor

Hotell commented Jun 28, 2021

one last CI fail -> missing config/test.js file (migration generator bug)

please create it with no content for now

↓↓↓

./config/tests.js

↓↓↓

/** Jest test setup file. */

},
coverageDirectory: './coverage',
setupFilesAfterEnv: ['./config/tests.js'],
snapshotSerializers: ['@fluentui/jest-serializer-make-styles'],
Copy link
Member

@layershifter layershifter Jun 28, 2021

Choose a reason for hiding this comment

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

It looks that there is a last CI failure:

ERR!   Module @fluentui/jest-serializer-make-styles in the snapshotSerializers option was not found.
ERR!          <rootDir> is: /mnt/work/1/s/packages/react-make-style

It happens because this package does not have @fluentui/jest-serializer-make-styles in dependencies/devDependencies. However it even does not need it, it is safe to delete this line:

Suggested change
snapshotSerializers: ['@fluentui/jest-serializer-make-styles'],

After that CI should be green 🟢


An issue that tracks it #18706.

Copy link
Contributor

Choose a reason for hiding this comment

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

yup, thx 🙌

@@ -51,7 +51,6 @@
"@fluentui/react-focus": "^8.1.5",
"@fluentui/react-hooks": "^8.2.3",
"@fluentui/react-icons-mdl2": "^1.1.4",
"@fluentui/react-make-styles": "^9.0.0-alpha.42",
Copy link
Contributor

@Hotell Hotell Jun 28, 2021

Choose a reason for hiding this comment

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

allright looks like we'll need to keep this here until all remaining packages are not migrated -> tracking the issue here #18752

@layershifter layershifter merged commit 804ca93 into microsoft:master Jun 29, 2021
@TristanWatanabe TristanWatanabe deleted the migrate-react-makestyles branch June 29, 2021 14:06
@msft-fluent-ui-bot
Copy link
Collaborator

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

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

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

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

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

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

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

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

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

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

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

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

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

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

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

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

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

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

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

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

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

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

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

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

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

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

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

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

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

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

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

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

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

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

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

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

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

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

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

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

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

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

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

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

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

Handy links:

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.

migration: migrate converged packages to new DX (1st stage)
6 participants