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: enable ESLint plugin for Griffel #22961

Merged

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented May 12, 2022

This PR adds new ESLint plugin for Griffel (microsoft/griffel#99).

Griffel does not support usage of CSS shorthands, microsoft/griffel#30.
This is enforced by typings, but there are cases when they don't work as expected microsoft/griffel#78.

Examples of incorrect code for this rule:

const useStyles = makeStyles({
  root: {
    // Causes cryptic TypeScript error
    background: "red"
  }
});

Current Behavior

No lint errors 😕

New Behavior

packages/react-components/react-link/src/components/Link/useLinkStyles.ts
  28:5  error  Usage of shorthands is prohibited  @griffel/no-shorthands

✖ 1 problem (1 error, 0 warnings)

@codesandbox-ci
Copy link

codesandbox-ci bot commented May 12, 2022

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

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

@size-auditor
Copy link

size-auditor bot commented May 12, 2022

Asset size changes

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

Baseline commit: 0915feeaa4278c32a7818b67b159ed207d1031fc (build)

@fabricteam
Copy link
Collaborator

fabricteam commented May 12, 2022

Perf Analysis (@fluentui/react-components)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 1246 1263 5000
Button mount 749 756 5000
FluentProvider mount 2280 2368 5000
FluentProviderWithTheme mount 393 423 10
FluentProviderWithTheme virtual-rerender 360 347 10
FluentProviderWithTheme virtual-rerender-with-unmount 451 445 10
MakeStyles mount 1910 1921 50000

@fabricteam
Copy link
Collaborator

fabricteam commented May 12, 2022

📊 Bundle size report

Unchanged fixtures
Package & Exports Size (minified/GZIP)
priority-overflow
createOverflowManager
2.936 kB
1.212 kB
react-accordion
Accordion (including children components)
73.795 kB
22.514 kB
react-avatar
Avatar
45.225 kB
13.142 kB
react-badge
Badge
20.992 kB
6.619 kB
react-badge
CounterBadge
21.903 kB
6.927 kB
react-badge
PresenceBadge
22.036 kB
6.593 kB
react-button
Button
32.475 kB
9.017 kB
react-button
CompoundButton
38.715 kB
10.169 kB
react-button
MenuButton
34.269 kB
9.602 kB
react-button
SplitButton
41.551 kB
10.907 kB
react-button
ToggleButton
43.403 kB
9.945 kB
react-card
Card - All
58.983 kB
16.92 kB
react-card
Card
54.268 kB
15.663 kB
react-card
CardFooter
7.783 kB
3.313 kB
react-card
CardHeader
9.351 kB
3.832 kB
react-card
CardPreview
7.755 kB
3.337 kB
react-combobox
Combobox
60.569 kB
20.68 kB
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
179.701 kB
50.147 kB
react-components
react-components: FluentProvider & webLightTheme
34.066 kB
11.089 kB
react-divider
Divider
15.484 kB
5.581 kB
react-image
Image
10.206 kB
4.009 kB
react-input
Input
22.522 kB
7.313 kB
react-label
Label
8.536 kB
3.564 kB
react-link
Link
11.412 kB
4.64 kB
react-menu
Menu (including children components)
110.223 kB
33.539 kB
react-menu
Menu (including selectable components)
113.398 kB
34.005 kB
react-overflow
hooks only
10.792 kB
4.124 kB
react-popover
Popover
101.146 kB
30.837 kB
react-portal
Portal
6.272 kB
2.17 kB
react-positioning
usePopper
23.21 kB
8.084 kB
react-provider
FluentProvider
14.115 kB
5.29 kB
react-radio
Radio
29.411 kB
10.059 kB
react-radio
RadioGroup
13.639 kB
5.475 kB
react-select
Select
17.064 kB
6.357 kB
react-slider
Slider
25.542 kB
8.258 kB
react-spinbutton
SpinButton
42.806 kB
11.985 kB
react-spinner
Spinner
17.321 kB
5.88 kB
react-switch
Switch
25.342 kB
8.219 kB
react-text
Text - Default
10.894 kB
4.285 kB
react-text
Text - Wrappers
14.212 kB
4.707 kB
react-textarea
Textarea
21.206 kB
7.17 kB
react-theme
Single theme token import
69 B
89 B
react-theme
Teams: all themes
31.363 kB
7.043 kB
react-theme
Teams: Light theme
19.806 kB
5.699 kB
react-tooltip
Tooltip
42.927 kB
14.775 kB
react-utilities
SSRProvider
189 B
161 B
🤖 This report was generated against 0915feeaa4278c32a7818b67b159ed207d1031fc

@fabricteam
Copy link
Collaborator

fabricteam commented May 12, 2022

Perf Analysis (@fluentui/react-northstar)

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
ListCommonPerf.default 674 605 1.11:1
TableMinimalPerf.default 419 383 1.09:1
HeaderMinimalPerf.default 358 331 1.08:1
ListNestedPerf.default 588 545 1.08:1
ReactionMinimalPerf.default 391 363 1.08:1
AlertMinimalPerf.default 274 256 1.07:1
ProviderMinimalPerf.default 414 387 1.07:1
GridMinimalPerf.default 338 320 1.06:1
MenuMinimalPerf.default 883 836 1.06:1
CarouselMinimalPerf.default 486 463 1.05:1
RadioGroupMinimalPerf.default 444 422 1.05:1
ChatMinimalPerf.default 743 717 1.04:1
DropdownManyItemsPerf.default 685 661 1.04:1
LabelMinimalPerf.default 392 377 1.04:1
PortalMinimalPerf.default 174 167 1.04:1
ToolbarMinimalPerf.default 980 940 1.04:1
TreeWith60ListItems.default 171 165 1.04:1
ChatWithPopoverPerf.default 388 378 1.03:1
FlexMinimalPerf.default 282 275 1.03:1
HeaderSlotsPerf.default 744 725 1.03:1
ImageMinimalPerf.default 373 363 1.03:1
InputMinimalPerf.default 1313 1272 1.03:1
PopupMinimalPerf.default 627 610 1.03:1
SkeletonMinimalPerf.default 333 323 1.03:1
ButtonSlotsPerf.default 569 560 1.02:1
DropdownMinimalPerf.default 3091 3034 1.02:1
ItemLayoutMinimalPerf.default 1205 1184 1.02:1
StatusMinimalPerf.default 669 658 1.02:1
TableManyItemsPerf.default 1975 1929 1.02:1
AvatarMinimalPerf.default 184 182 1.01:1
RefMinimalPerf.default 234 231 1.01:1
TextMinimalPerf.default 329 326 1.01:1
TreeMinimalPerf.default 792 783 1.01:1
AttachmentMinimalPerf.default 143 143 1:1
FormMinimalPerf.default 390 390 1:1
LayoutMinimalPerf.default 352 351 1:1
ListMinimalPerf.default 493 492 1:1
ListWith60ListItems.default 640 639 1:1
LoaderMinimalPerf.default 674 675 1:1
SegmentMinimalPerf.default 336 337 1:1
SliderMinimalPerf.default 1708 1706 1:1
SplitButtonMinimalPerf.default 4412 4403 1:1
AttachmentSlotsPerf.default 1074 1086 0.99:1
ButtonMinimalPerf.default 168 169 0.99:1
ButtonOverridesMissPerf.default 1545 1558 0.99:1
DatepickerMinimalPerf.default 5761 5843 0.99:1
EmbedMinimalPerf.default 4149 4193 0.99:1
RosterPerf.default 1112 1118 0.99:1
TextAreaMinimalPerf.default 471 475 0.99:1
AnimationMinimalPerf.default 555 569 0.98:1
CardMinimalPerf.default 549 560 0.98:1
ChatDuplicateMessagesPerf.default 283 288 0.98:1
CheckboxMinimalPerf.default 2655 2703 0.98:1
MenuButtonMinimalPerf.default 1686 1721 0.98:1
CustomToolbarPrototype.default 2709 2754 0.98:1
VideoMinimalPerf.default 618 629 0.98:1
AccordionMinimalPerf.default 140 144 0.97:1
ProviderMergeThemesPerf.default 1240 1278 0.97:1
TooltipMinimalPerf.default 1089 1119 0.97:1
DialogMinimalPerf.default 750 783 0.96:1
BoxMinimalPerf.default 333 351 0.95:1
DividerMinimalPerf.default 335 352 0.95:1
IconMinimalPerf.default 575 615 0.93:1

@fabricteam
Copy link
Collaborator

fabricteam commented May 12, 2022

Perf Analysis (@fluentui/react)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
BaseButton mount 1021 1046 5000
Breadcrumb mount 2919 2896 1000
Checkbox mount 1606 1698 5000
CheckboxBase mount 1384 1396 5000
ChoiceGroup mount 5149 5093 5000
ComboBox mount 1044 1030 1000
CommandBar mount 10937 10840 1000
ContextualMenu mount 11395 11384 1000
DefaultButton mount 1228 1246 5000
DetailsRow mount 4104 4181 5000
DetailsRowFast mount 4144 4071 5000
DetailsRowNoStyles mount 3867 3857 5000
Dialog mount 2474 2485 1000
DocumentCardTitle mount 192 201 1000
Dropdown mount 3547 3580 5000
FocusTrapZone mount 1935 1879 5000
FocusZone mount 1870 1852 5000
IconButton mount 2004 1912 5000
Label mount 374 382 5000
Layer mount 3153 3214 5000
Link mount 495 509 5000
MenuButton mount 1617 1624 5000
MessageBar mount 2286 2153 5000
Nav mount 3566 3607 1000
OverflowSet mount 1150 1216 5000
Panel mount 2403 2333 1000
Persona mount 1147 1167 1000
Pivot mount 1673 1648 1000
PrimaryButton mount 1506 1509 5000
Rating mount 8823 8777 5000
SearchBox mount 1451 1519 5000
Shimmer mount 2768 2739 5000
Slider mount 2048 2086 5000
SpinButton mount 5370 5337 5000
Spinner mount 453 468 5000
SplitButton mount 3379 3348 5000
Stack mount 586 564 5000
StackWithIntrinsicChildren mount 2694 2676 5000
StackWithTextChildren mount 5963 5918 5000
SwatchColorPicker mount 12277 12594 5000
TagPicker mount 2917 2917 5000
TeachingBubble mount 87840 89882 5000
Text mount 460 454 5000
TextField mount 1545 1508 5000
ThemeProvider mount 1240 1262 5000
ThemeProvider virtual-rerender 665 665 5000
ThemeProvider virtual-rerender-with-unmount 2109 2077 5000
Toggle mount 874 917 5000
buttonNative mount 132 137 5000

@layershifter layershifter marked this pull request as ready for review May 12, 2022 11:38
@layershifter layershifter requested a review from a team as a code owner May 12, 2022 11:38
@@ -10,5 +10,6 @@ module.exports = {
rules: {
...configHelpers.getNamingConventionRule(true /* prefixWithI */),
'jsdoc/check-tag-names': 'off',
'@griffel/no-shorthands': 'off',
Copy link
Member Author

Choose a reason for hiding this comment

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

v8 has own makeStyles, new rule collides with it.

Copy link
Member

Choose a reason for hiding this comment

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

not related to this PR, but what are partners supposed to do in their repo when they have both v8 and v9?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, I am not sure that makeStyles in v8 is a recommended approach:

* @deprecated Use `mergeStyles` instead for v8 related code. We will be using a new implementation of `makeStyles` in
* future versions of the library.

So for that usages that probably just ignore files for that rule.

Copy link
Contributor

Choose a reason for hiding this comment

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

wished I merged this earlier #22128

I'll update that PR with these changes. So we wanna have it enabled only for this config is that correct ?

image

Copy link
Contributor

Choose a reason for hiding this comment

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

not related to this PR, but what are partners supposed to do in their repo when they have both v8 and v9?

I'd say it should be up to them to enable linting rules that they need. our monorepo eslint plugin/config is not in the best shape and we don't follow proper semver for it, so I would not recommend to any partner to use our configurations

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 update that PR with these changes. So we wanna have it enabled only for this config is that correct ?

Yes, only for v9 packages 👍

@tudorpopams tudorpopams requested a review from Hotell May 12, 2022 12:21
@@ -10,5 +10,6 @@ module.exports = {
rules: {
...configHelpers.getNamingConventionRule(true /* prefixWithI */),
'jsdoc/check-tag-names': 'off',
'@griffel/no-shorthands': 'off',
Copy link
Contributor

Choose a reason for hiding this comment

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

wished I merged this earlier #22128

I'll update that PR with these changes. So we wanna have it enabled only for this config is that correct ?

image

yarn.lock Show resolved Hide resolved
@@ -10,5 +10,6 @@ module.exports = {
rules: {
...configHelpers.getNamingConventionRule(true /* prefixWithI */),
'jsdoc/check-tag-names': 'off',
'@griffel/no-shorthands': 'off',
Copy link
Contributor

Choose a reason for hiding this comment

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

not related to this PR, but what are partners supposed to do in their repo when they have both v8 and v9?

I'd say it should be up to them to enable linting rules that they need. our monorepo eslint plugin/config is not in the best shape and we don't follow proper semver for it, so I would not recommend to any partner to use our configurations

@layershifter layershifter merged commit 4b1c0d1 into microsoft:master May 12, 2022
@layershifter layershifter deleted the chore/enable-griffel-plugin branch May 12, 2022 16:35
marwan38 pushed a commit to marwan38/fluentui that referenced this pull request Jun 13, 2022
* chore: enable ESLint plugin for Griffel

* add change file

* fix config for legacy
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