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(make-styles): migrate to new DX #18673

Merged
merged 7 commits into from
Jun 25, 2021
Merged

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented Jun 23, 2021

Pull request checklist

Description of changes

Migration of package to new DX.

Changes that were done:

  • .storybook directory as scripts were removed as this package does not have stories
  • tsconfig.json#compilerOptions.lib setting was modified to previous as this package uses ES2015 features
  • jest.config.js#snapshotSerializers & jest.config.js#setupFilesAfterEnv settings were removed

@size-auditor
Copy link

size-auditor bot commented Jun 23, 2021

Asset size changes

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

Baseline commit: 5a6dd90efb20b162aa3883c34faa211b5620b334 (build)

@layershifter
Copy link
Member Author

I tested commands after migration and it looks that build:local does not work as expected 😥 I created an issue #18675.

@fabricteam
Copy link
Collaborator

fabricteam commented Jun 23, 2021

Perf Analysis (@fluentui/react)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 805 825 5000
BaseButton mount 949 896 5000
Breadcrumb mount 2616 2643 1000
ButtonNext mount 505 513 5000
Checkbox mount 1531 1474 5000
CheckboxBase mount 1252 1252 5000
ChoiceGroup mount 4603 4642 5000
ComboBox mount 1063 992 1000
CommandBar mount 10099 10184 1000
ContextualMenu mount 6339 6215 1000
DefaultButton mount 1127 1155 5000
DetailsRow mount 3700 3652 5000
DetailsRowFast mount 3794 3711 5000
DetailsRowNoStyles mount 3425 3507 5000
Dialog mount 2121 2103 1000
DocumentCardTitle mount 132 148 1000
Dropdown mount 3244 3134 5000
FocusTrapZone mount 1767 1791 5000
FocusZone mount 1740 1754 5000
IconButton mount 1661 1754 5000
Label mount 351 340 5000
Layer mount 1750 1794 5000
Link mount 477 449 5000
MakeStyles mount 1813 1833 50000
MenuButton mount 1415 1447 5000
MessageBar mount 2022 1993 5000
Nav mount 3147 3175 1000
OverflowSet mount 1041 1010 5000
Panel mount 2032 2030 1000
Persona mount 789 788 1000
Pivot mount 1396 1366 1000
PrimaryButton mount 1272 1263 5000
Rating mount 7578 7449 5000
SearchBox mount 1324 1312 5000
Shimmer mount 2488 2477 5000
Slider mount 1923 1930 5000
SpinButton mount 4841 4878 5000
Spinner mount 413 404 5000
SplitButton mount 3152 3077 5000
Stack mount 511 501 5000
StackWithIntrinsicChildren mount 1537 1559 5000
StackWithTextChildren mount 4453 4465 5000
SwatchColorPicker mount 10167 10090 5000
Tabs mount 1402 1370 1000
TagPicker mount 2411 2426 5000
TeachingBubble mount 11705 11679 5000
Text mount 426 403 5000
TextField mount 1349 1340 5000
ThemeProvider mount 1166 1184 5000
ThemeProvider virtual-rerender 601 593 5000
ThemeProviderNext mount 7098 7117 5000
Toggle mount 792 767 5000
buttonNative mount 120 109 5000

Perf Analysis (@fluentui/react-northstar)

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
AttachmentMinimalPerf.default 155 141 1.1:1
IconMinimalPerf.default 611 564 1.08:1
ListWith60ListItems.default 661 619 1.07:1
CarouselMinimalPerf.default 466 439 1.06:1
ListCommonPerf.default 639 601 1.06:1
ListMinimalPerf.default 525 500 1.05:1
TableMinimalPerf.default 407 387 1.05:1
AlertMinimalPerf.default 290 278 1.04:1
AvatarMinimalPerf.default 199 191 1.04:1
DialogMinimalPerf.default 767 738 1.04:1
HeaderSlotsPerf.default 763 732 1.04:1
ChatDuplicateMessagesPerf.default 289 280 1.03:1
DropdownManyItemsPerf.default 677 659 1.03:1
MenuMinimalPerf.default 848 823 1.03:1
SkeletonMinimalPerf.default 352 343 1.03:1
StatusMinimalPerf.default 658 641 1.03:1
TooltipMinimalPerf.default 977 944 1.03:1
ChatMinimalPerf.default 646 631 1.02:1
CheckboxMinimalPerf.default 2711 2661 1.02:1
DatepickerMinimalPerf.default 5313 5197 1.02:1
ImageMinimalPerf.default 364 358 1.02:1
PortalMinimalPerf.default 173 170 1.02:1
RadioGroupMinimalPerf.default 453 443 1.02:1
SegmentMinimalPerf.default 345 339 1.02:1
SliderMinimalPerf.default 1566 1528 1.02:1
TextMinimalPerf.default 341 334 1.02:1
AccordionMinimalPerf.default 146 144 1.01:1
AttachmentSlotsPerf.default 1034 1022 1.01:1
DropdownMinimalPerf.default 3029 3012 1.01:1
ItemLayoutMinimalPerf.default 1188 1171 1.01:1
LabelMinimalPerf.default 373 370 1.01:1
PopupMinimalPerf.default 558 555 1.01:1
ReactionMinimalPerf.default 363 361 1.01:1
SplitButtonMinimalPerf.default 3682 3656 1.01:1
ButtonMinimalPerf.default 158 158 1:1
ChatWithPopoverPerf.default 355 354 1:1
DividerMinimalPerf.default 349 350 1:1
FlexMinimalPerf.default 276 277 1:1
GridMinimalPerf.default 332 333 1:1
HeaderMinimalPerf.default 343 343 1:1
LoaderMinimalPerf.default 681 684 1:1
ProviderMinimalPerf.default 992 996 1:1
CustomToolbarPrototype.default 3747 3763 1:1
TreeMinimalPerf.default 765 764 1:1
AnimationMinimalPerf.default 402 406 0.99:1
BoxMinimalPerf.default 343 345 0.99:1
ButtonOverridesMissPerf.default 1638 1662 0.99:1
EmbedMinimalPerf.default 4071 4094 0.99:1
FormMinimalPerf.default 389 391 0.99:1
LayoutMinimalPerf.default 353 355 0.99:1
ListNestedPerf.default 541 545 0.99:1
MenuButtonMinimalPerf.default 1541 1559 0.99:1
ProviderMergeThemesPerf.default 1631 1642 0.99:1
TableManyItemsPerf.default 1872 1888 0.99:1
ToolbarMinimalPerf.default 907 916 0.99:1
CardMinimalPerf.default 536 547 0.98:1
InputMinimalPerf.default 1247 1277 0.98:1
ButtonSlotsPerf.default 536 555 0.97:1
RefMinimalPerf.default 223 230 0.97:1
TextAreaMinimalPerf.default 459 471 0.97:1
TreeWith60ListItems.default 171 179 0.96:1
VideoMinimalPerf.default 595 622 0.96:1
RosterPerf.default 1102 1172 0.94:1

@Hotell
Copy link
Contributor

Hotell commented Jun 23, 2021

tsconfig.json#compilerOptions.lib setting was modified to previous as this package uses ES2015 features

@ling1726 can you reference this PR as well in the issue -> It touches what we talked about today regarding language feature parity misalignment. thx!

@Hotell
Copy link
Contributor

Hotell commented Jun 23, 2021

jest.config.js#snapshotSerializers & jest.config.js#setupFilesAfterEnv settings were removed

would you be willing to implement those behaviours (to not include those everywhere + not include storybook when its a node package) to the generator ?

@@ -71,7 +71,7 @@
"@fluentui/jest-serializer-make-styles": { "implicitDependencies": [] },
"@fluentui/jest-serializer-merge-styles": { "implicitDependencies": [] },
"@fluentui/keyboard-key": { "implicitDependencies": [] },
"@fluentui/make-styles": { "implicitDependencies": [] },
"@fluentui/make-styles": { "tags": ["vNext", "platform:web"], "implicitDependencies": [] },
Copy link
Contributor

Choose a reason for hiding this comment

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

to double check the context boundary, is this package solely for web platform or it can be used also within node ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a web package, some functionality is related to SSR, but I am not considering this usage as within Node.

Copy link
Contributor

Choose a reason for hiding this comment

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

kk, for better context

  • platform:web - package used on web
  • platform:node - package used with node
  • platform:any - package that can be used in both

We'll enable lint rule that will respect those contexts and staticly defined rules in the future to prevent for example importing node package into web package

@layershifter
Copy link
Member Author

would you be willing to implement those behaviours (to not include those everywhere + not include storybook when its a node package) to the generator ?

For jest.config.js

I don't think that it's related to node/web, I created an issue #18706, please take a look when you will have time.

For storybook

Honestly I am not entirely sure about this. For example, this package will not have stories ever as all of them are in @fluentui/react-make-styles that wraps core functionality from this package. There are only two packages Node-only packages (babel-make-styles and jest-serializer-make-styles), IMO during migration of them we can just delete .storybook and related commands for them. WDYT?

@Hotell
Copy link
Contributor

Hotell commented Jun 25, 2021

For storybook

While I agree in general, to provide best possible DX for folks migrating this would help a lot without additional cognitive load needed.

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.

🙌 thx!

…hore/make-styles-dx

� Conflicts:
�	jest.config.js
@layershifter layershifter enabled auto-merge (squash) June 25, 2021 11:22
@layershifter layershifter merged commit bf6fd12 into master Jun 25, 2021
@layershifter layershifter deleted the chore/make-styles-dx branch June 25, 2021 12:25
@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.

tools: after nx migration build:local does not work
6 participants