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 migration for @fluentui/make-styles #20742

Merged
merged 7 commits into from
Nov 26, 2021
Merged

Conversation

layershifter
Copy link
Member

Pull request checklist

Description of changes

@fabricteam
Copy link
Collaborator

fabricteam commented Nov 24, 2021

📊 Bundle size report

🤖 This report was generated against 5a49512b8e33d5a588212069a1f2827b806d584c

@layershifter
Copy link
Member Author

layershifter commented Nov 24, 2021

Changes applied by generator are causing build failure:

image

As packages/make-styles/src/utils/test/snapshotSerializer.ts uses jest namespace for types.

image

import type { SnapshotSerializerPlugin } from 'jest' 

Is not possible as it's not exported. @Hotell, @ling1726 any ideas?

Solved 👇

@ling1726
Copy link
Member

Changes applied by generator are causing build failure:

image

As packages/make-styles/src/utils/test/snapshotSerializer.ts uses jest namespace for types.

image

import type { SnapshotSerializerPlugin } from 'jest' 

Is not possible as it's not exported. @Hotell, @ling1726 any ideas?

IMO this is a case where having jest types in tsconfig.lib.json is justified... because in that case it's not leaking types :)

@size-auditor
Copy link

size-auditor bot commented Nov 24, 2021

Asset size changes

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

Baseline commit: ecdea9fb8c041b82997e1b6f6b8be49882dbd9f2 (build)

@layershifter layershifter added the Status: Blocked Resolution blocked by another issue label Nov 24, 2021
@layershifter layershifter requested a review from a team November 25, 2021 16:50
@ling1726
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@Hotell
Copy link
Contributor

Hotell commented Nov 26, 2021

to get a proper solution, can you give us pls context what's the purpose of this module ?

2021-11-26 at 12 41

Update:
I don't see it being used at all, nor exported -> after removing explicit jest type, VSCode showed usage properly
2021-11-26 at 12 46

So if I understand this correctly, those are helpers solely for testing. if that's the case currently those live in src/common.

once you'll move it there you should be good to go.

One more thing -> you'll need to add following glob to tsconfig.spec.json include field ./src/common/**

@layershifter
Copy link
Member Author

to get a proper solution, can you give us pls context what's the purpose of this module ?

2021-11-26 at 12 41

I don't see it being used at all, nor exported

@Hotell It's used internally in other tests:

import { makeStylesRendererSerializer } from './utils/test/snapshotSerializer';
import { makeStyles } from './makeStyles';
import { MakeStylesRenderer } from './types';
expect.addSnapshotSerializer(makeStylesRendererSerializer);

It should not be exported.

@Hotell
Copy link
Contributor

Hotell commented Nov 26, 2021

updated original message ^ , thx!

@layershifter
Copy link
Member Author

layershifter commented Nov 26, 2021

to get a proper solution, can you give us pls context what's the purpose of this module ?

2021-11-26 at 12 41

Update: I don't see it being used at all, nor exported -> after removing explicit jest type, VSCode showed usage properly 2021-11-26 at 12 46

So if I understand this correctly, those are helpers solely for testing. if that's the case currently those live in src/common.

once you'll move it there you should be good to go.

One more thing -> you'll need to add following glob to tsconfig.spec.json include field ./src/common/**

@Hotell should I also exclude it in tsconfig.lib.json? If so, common does not have much sense 🤔

Otherwise it fails:

image

@Hotell
Copy link
Contributor

Hotell commented Nov 26, 2021

@Hotell should I also exclude it in tsconfig.lib.json? If so, common does not have much sense 🤔

yes pls,
2021-11-26 at 14 14

I'll update the generator so it will do it for you next time. thx!

@layershifter layershifter removed the Status: Blocked Resolution blocked by another issue label Nov 26, 2021
@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 26, 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 3d52dea:

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

@@ -7,7 +7,7 @@ const jju = require('jju');
const testFiles = [
'**/*{.,-}test.{ts,tsx}',
'**/*.stories.tsx',
'**/{test,tests,stories}/**',
'**/{common,test,tests,stories}/**',
Copy link
Member Author

@layershifter layershifter Nov 26, 2021

Choose a reason for hiding this comment

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

Had to add this to solve lint issue:

image

If it's not acceptable, I can add override in local eslintrc. @Hotell WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think its ok. how did you get this error ? on CI when running lint task for make-styles?

Copy link
Member Author

Choose a reason for hiding this comment

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

On CI with lint command, was able to repro it locally, too

@@ -2,7 +2,6 @@ import { MakeStylesRenderer, StyleBucketName } from '../types';

// Regexps to extract names of classes and animations
// https://github.com/styletron/styletron/blob/e0fcae826744eb00ce679ac613a1b10d44256660/packages/styletron-engine-atomic/src/client/client.js#L8
// eslint-disable-next-line @fluentui/max-len
Copy link
Member Author

Choose a reason for hiding this comment

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

Was not used ¯_(ツ)_/¯

@fabricteam
Copy link
Collaborator

fabricteam commented Nov 26, 2021

Perf Analysis (@fluentui/react)

Scenario Render type Master Ticks PR Ticks Iterations Status
ContextualMenu mount 9245 16247 1000 Possible regression
All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 1194 1205 5000
BaseButton mount 1199 1261 5000
Breadcrumb mount 2903 2907 1000
ButtonNext mount 675 736 5000
Checkbox mount 1782 1762 5000
CheckboxBase mount 1652 1601 5000
ChoiceGroup mount 5272 5277 5000
ComboBox mount 1207 1114 1000
CommandBar mount 10538 11627 1000
ContextualMenu mount 9245 16247 1000 Possible regression
DefaultButton mount 1421 1474 5000
DetailsRow mount 4125 4142 5000
DetailsRowFast mount 4143 4095 5000
DetailsRowNoStyles mount 3932 3898 5000
Dialog mount 2844 2817 1000
DocumentCardTitle mount 336 282 1000
Dropdown mount 3579 3546 5000
FluentProviderNext mount 4097 4027 5000
FluentProviderWithTheme mount 398 432 10
FluentProviderWithTheme virtual-rerender 252 275 10
FluentProviderWithTheme virtual-rerender-with-unmount 450 449 10
FocusTrapZone mount 2031 2104 5000
FocusZone mount 2014 2010 5000
IconButton mount 2102 2213 5000
Label mount 529 517 5000
Layer mount 3295 3219 5000
Link mount 679 656 5000
MakeStyles mount 1993 1964 50000
MenuButton mount 1736 1735 5000
MessageBar mount 2259 2184 5000
Nav mount 3814 3726 1000
OverflowSet mount 1390 1361 5000
Panel mount 2693 2768 1000
Persona mount 1040 1053 1000
Pivot mount 1657 1638 1000
PrimaryButton mount 1535 1593 5000
Rating mount 8630 8713 5000
SearchBox mount 1679 1610 5000
Shimmer mount 3031 3052 5000
Slider mount 2246 2219 5000
SpinButton mount 5657 5506 5000
Spinner mount 595 591 5000
SplitButton mount 3509 3484 5000
Stack mount 739 697 5000
StackWithIntrinsicChildren mount 2049 2068 5000
StackWithTextChildren mount 5490 5576 5000
SwatchColorPicker mount 11263 11272 5000
TagPicker mount 2899 2977 5000
TeachingBubble mount 14053 13608 5000
Text mount 638 652 5000
TextField mount 1653 1696 5000
ThemeProvider mount 1385 1367 5000
ThemeProvider virtual-rerender 752 790 5000
ThemeProvider virtual-rerender-with-unmount 2144 2159 5000
Toggle mount 1010 1042 5000
buttonNative mount 303 291 5000

Perf Analysis (@fluentui/react-northstar)

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
GridMinimalPerf.default 385 341 1.13:1
SkeletonMinimalPerf.default 412 365 1.13:1
AttachmentMinimalPerf.default 195 176 1.11:1
TextMinimalPerf.default 411 377 1.09:1
ChatMinimalPerf.default 735 685 1.07:1
DropdownManyItemsPerf.default 805 749 1.07:1
ItemLayoutMinimalPerf.default 1382 1296 1.07:1
AlertMinimalPerf.default 331 312 1.06:1
RefMinimalPerf.default 246 232 1.06:1
ButtonSlotsPerf.default 610 583 1.05:1
CardMinimalPerf.default 628 597 1.05:1
DividerMinimalPerf.default 406 386 1.05:1
ListCommonPerf.default 714 679 1.05:1
ListWith60ListItems.default 690 660 1.05:1
ToolbarMinimalPerf.default 1090 1038 1.05:1
TreeWith60ListItems.default 203 194 1.05:1
HeaderMinimalPerf.default 421 405 1.04:1
ImageMinimalPerf.default 448 431 1.04:1
MenuMinimalPerf.default 913 877 1.04:1
PortalMinimalPerf.default 182 175 1.04:1
ReactionMinimalPerf.default 405 390 1.04:1
StatusMinimalPerf.default 729 704 1.04:1
AccordionMinimalPerf.default 185 179 1.03:1
ChatDuplicateMessagesPerf.default 317 308 1.03:1
InputMinimalPerf.default 1450 1409 1.03:1
LabelMinimalPerf.default 408 396 1.03:1
RadioGroupMinimalPerf.default 490 477 1.03:1
TooltipMinimalPerf.default 1130 1099 1.03:1
AttachmentSlotsPerf.default 1194 1173 1.02:1
BoxMinimalPerf.default 406 399 1.02:1
ButtonOverridesMissPerf.default 1844 1803 1.02:1
DropdownMinimalPerf.default 3063 3008 1.02:1
LayoutMinimalPerf.default 397 390 1.02:1
ListMinimalPerf.default 546 534 1.02:1
LoaderMinimalPerf.default 712 695 1.02:1
SplitButtonMinimalPerf.default 4655 4585 1.02:1
IconMinimalPerf.default 666 654 1.02:1
TableMinimalPerf.default 466 459 1.02:1
AnimationMinimalPerf.default 477 472 1.01:1
ButtonMinimalPerf.default 191 189 1.01:1
EmbedMinimalPerf.default 4430 4400 1.01:1
SliderMinimalPerf.default 1756 1734 1.01:1
CarouselMinimalPerf.default 499 499 1:1
CheckboxMinimalPerf.default 2776 2765 1:1
DatepickerMinimalPerf.default 5681 5653 1:1
FormMinimalPerf.default 473 473 1:1
MenuButtonMinimalPerf.default 1739 1742 1:1
ProviderMergeThemesPerf.default 1680 1672 1:1
TextAreaMinimalPerf.default 601 603 1:1
AvatarMinimalPerf.default 230 232 0.99:1
FlexMinimalPerf.default 327 331 0.99:1
HeaderSlotsPerf.default 860 872 0.99:1
PopupMinimalPerf.default 608 612 0.99:1
TableManyItemsPerf.default 2165 2177 0.99:1
TreeMinimalPerf.default 833 840 0.99:1
VideoMinimalPerf.default 669 676 0.99:1
DialogMinimalPerf.default 814 831 0.98:1
ListNestedPerf.default 576 588 0.98:1
RosterPerf.default 1306 1331 0.98:1
ProviderMinimalPerf.default 1158 1183 0.98:1
CustomToolbarPrototype.default 4503 4649 0.97:1
ChatWithPopoverPerf.default 398 414 0.96:1
SegmentMinimalPerf.default 369 388 0.95:1

@layershifter layershifter merged commit 7c5dc03 into master Nov 26, 2021
@layershifter layershifter deleted the chore/migrate-mk branch November 26, 2021 17:17
mlp73 pushed a commit to mlp73/fluentui that referenced this pull request Jan 17, 2022
* chore: run migration for @fluentui/make-styles

* Change files

* move serializer to common

* remove unused eslint-disable

* add common to "testFiles"

* Change files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants