-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
feat: create core/base for react/node/legacy lint configs #22128
feat: create core/base for react/node/legacy lint configs #22128
Conversation
root: true, | ||
extends: [ | ||
// Provides both rules and some parser options and other settings | ||
'airbnb', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as follow up should be removed / tweaked only for legacy
const configHelpers = require('../utils/configHelpers'); | ||
|
||
/** @type {import("eslint").Linter.Config} */ | ||
module.exports = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
config that handles only react family of plugins
@@ -1,440 +1,42 @@ | |||
// @ts-check | |||
const configHelpers = require('../utils/configHelpers'); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
contains only rules applied in v9 react packages
@@ -5,7 +5,7 @@ const configHelpers = require('../utils/configHelpers'); | |||
|
|||
/** @type {import("eslint").Linter.Config} */ | |||
module.exports = { | |||
extends: [path.join(__dirname, 'react')], | |||
extends: [path.join(__dirname, 'core'), path.join(__dirname, 'react-config')], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
contains only rules applied in v7,8 react packages
@@ -0,0 +1,360 @@ | |||
// @ts-check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOTE: this whole config is copy-paste of original react.js
file , cleaned up by removing react family specific rules/plugins setup
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 7428691:
|
📊 Bundle size reportUnchanged fixtures
|
Perf Analysis (
|
Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
---|---|---|---|---|---|
Avatar | mount | 1079 | 1080 | 5000 | |
Button | mount | 672 | 704 | 5000 | |
FluentProvider | mount | 2204 | 2223 | 5000 | |
FluentProviderWithTheme | mount | 355 | 350 | 10 | |
FluentProviderWithTheme | virtual-rerender | 298 | 311 | 10 | |
FluentProviderWithTheme | virtual-rerender-with-unmount | 357 | 370 | 10 | |
MakeStyles | mount | 1928 | 1876 | 50000 |
Asset size changesUnable to find bundle size details for Baseline commit: 96f4f78 Possible causes
Recommendations
|
Perf Analysis (
|
Scenario | Current PR Ticks | Baseline Ticks | Ratio |
---|---|---|---|
ChatDuplicateMessagesPerf.default | 372 | 311 | 1.2:1 |
RefMinimalPerf.default | 274 | 238 | 1.15:1 |
SegmentMinimalPerf.default | 420 | 375 | 1.12:1 |
FormMinimalPerf.default | 532 | 478 | 1.11:1 |
ProviderMinimalPerf.default | 451 | 410 | 1.1:1 |
TextMinimalPerf.default | 403 | 369 | 1.09:1 |
TextAreaMinimalPerf.default | 627 | 577 | 1.09:1 |
MenuMinimalPerf.default | 1016 | 938 | 1.08:1 |
PortalMinimalPerf.default | 188 | 174 | 1.08:1 |
AttachmentMinimalPerf.default | 188 | 176 | 1.07:1 |
TreeWith60ListItems.default | 204 | 191 | 1.07:1 |
AttachmentSlotsPerf.default | 1290 | 1216 | 1.06:1 |
ButtonMinimalPerf.default | 199 | 188 | 1.06:1 |
CarouselMinimalPerf.default | 532 | 504 | 1.06:1 |
LabelMinimalPerf.default | 446 | 421 | 1.06:1 |
TooltipMinimalPerf.default | 1395 | 1321 | 1.06:1 |
AccordionMinimalPerf.default | 176 | 167 | 1.05:1 |
AvatarMinimalPerf.default | 237 | 226 | 1.05:1 |
ChatWithPopoverPerf.default | 440 | 419 | 1.05:1 |
HeaderMinimalPerf.default | 433 | 414 | 1.05:1 |
ListWith60ListItems.default | 758 | 723 | 1.05:1 |
LoaderMinimalPerf.default | 768 | 728 | 1.05:1 |
MenuButtonMinimalPerf.default | 2019 | 1922 | 1.05:1 |
ProviderMergeThemesPerf.default | 1360 | 1306 | 1.04:1 |
CheckboxMinimalPerf.default | 3097 | 3018 | 1.03:1 |
EmbedMinimalPerf.default | 4724 | 4567 | 1.03:1 |
FlexMinimalPerf.default | 346 | 335 | 1.03:1 |
InputMinimalPerf.default | 1521 | 1479 | 1.03:1 |
StatusMinimalPerf.default | 831 | 803 | 1.03:1 |
AnimationMinimalPerf.default | 617 | 604 | 1.02:1 |
ButtonOverridesMissPerf.default | 1735 | 1707 | 1.02:1 |
ChatMinimalPerf.default | 874 | 860 | 1.02:1 |
DialogMinimalPerf.default | 859 | 842 | 1.02:1 |
SkeletonMinimalPerf.default | 426 | 416 | 1.02:1 |
DatepickerMinimalPerf.default | 6574 | 6512 | 1.01:1 |
ItemLayoutMinimalPerf.default | 1394 | 1376 | 1.01:1 |
SliderMinimalPerf.default | 1850 | 1838 | 1.01:1 |
SplitButtonMinimalPerf.default | 4983 | 4958 | 1.01:1 |
ListNestedPerf.default | 642 | 641 | 1:1 |
CustomToolbarPrototype.default | 2919 | 2918 | 1:1 |
DropdownManyItemsPerf.default | 788 | 794 | 0.99:1 |
DropdownMinimalPerf.default | 3268 | 3292 | 0.99:1 |
RadioGroupMinimalPerf.default | 506 | 512 | 0.99:1 |
TableManyItemsPerf.default | 2266 | 2291 | 0.99:1 |
TreeMinimalPerf.default | 934 | 942 | 0.99:1 |
ButtonSlotsPerf.default | 614 | 626 | 0.98:1 |
GridMinimalPerf.default | 371 | 380 | 0.98:1 |
ListCommonPerf.default | 761 | 774 | 0.98:1 |
PopupMinimalPerf.default | 681 | 693 | 0.98:1 |
TableMinimalPerf.default | 470 | 480 | 0.98:1 |
VideoMinimalPerf.default | 742 | 759 | 0.98:1 |
AlertMinimalPerf.default | 304 | 314 | 0.97:1 |
ToolbarMinimalPerf.default | 1076 | 1105 | 0.97:1 |
BoxMinimalPerf.default | 375 | 394 | 0.95:1 |
CardMinimalPerf.default | 650 | 684 | 0.95:1 |
ImageMinimalPerf.default | 459 | 485 | 0.95:1 |
RosterPerf.default | 1245 | 1311 | 0.95:1 |
ListMinimalPerf.default | 572 | 606 | 0.94:1 |
LayoutMinimalPerf.default | 389 | 418 | 0.93:1 |
IconMinimalPerf.default | 699 | 755 | 0.93:1 |
HeaderSlotsPerf.default | 879 | 967 | 0.91:1 |
ReactionMinimalPerf.default | 413 | 475 | 0.87:1 |
DividerMinimalPerf.default | 390 | 454 | 0.86:1 |
Perf Analysis (
|
Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
---|---|---|---|---|---|
BaseButton | mount | 879 | 917 | 5000 | |
Breadcrumb | mount | 2696 | 2688 | 1000 | |
Checkbox | mount | 1440 | 1481 | 5000 | |
CheckboxBase | mount | 1221 | 1226 | 5000 | |
ChoiceGroup | mount | 4719 | 4622 | 5000 | |
ComboBox | mount | 958 | 965 | 1000 | |
CommandBar | mount | 10302 | 10416 | 1000 | |
ContextualMenu | mount | 11157 | 11393 | 1000 | |
DefaultButton | mount | 1112 | 1138 | 5000 | |
DetailsRow | mount | 3772 | 3806 | 5000 | |
DetailsRowFast | mount | 3761 | 3744 | 5000 | |
DetailsRowNoStyles | mount | 3623 | 3570 | 5000 | |
Dialog | mount | 2238 | 2227 | 1000 | |
DocumentCardTitle | mount | 165 | 166 | 1000 | |
Dropdown | mount | 3192 | 3243 | 5000 | |
FocusTrapZone | mount | 1839 | 1793 | 5000 | |
FocusZone | mount | 1809 | 1822 | 5000 | |
IconButton | mount | 1731 | 1728 | 5000 | |
Label | mount | 340 | 354 | 5000 | |
Layer | mount | 2873 | 2958 | 5000 | |
Link | mount | 465 | 455 | 5000 | |
MenuButton | mount | 1489 | 1456 | 5000 | |
MessageBar | mount | 2173 | 2213 | 5000 | |
Nav | mount | 3429 | 3492 | 1000 | |
OverflowSet | mount | 1135 | 1172 | 5000 | |
Panel | mount | 2240 | 2265 | 1000 | |
Persona | mount | 1070 | 993 | 1000 | |
Pivot | mount | 1479 | 1535 | 1000 | |
PrimaryButton | mount | 1347 | 1355 | 5000 | |
Rating | mount | 7911 | 8066 | 5000 | |
SearchBox | mount | 1285 | 1314 | 5000 | |
Shimmer | mount | 2476 | 2474 | 5000 | |
Slider | mount | 1934 | 2005 | 5000 | |
SpinButton | mount | 5033 | 5035 | 5000 | |
Spinner | mount | 422 | 434 | 5000 | |
SplitButton | mount | 3173 | 3231 | 5000 | |
Stack | mount | 496 | 517 | 5000 | |
StackWithIntrinsicChildren | mount | 2255 | 2247 | 5000 | |
StackWithTextChildren | mount | 5184 | 5221 | 5000 | |
SwatchColorPicker | mount | 11537 | 11616 | 5000 | |
TagPicker | mount | 2710 | 2738 | 5000 | |
TeachingBubble | mount | 92709 | 93418 | 5000 | |
Text | mount | 434 | 439 | 5000 | |
TextField | mount | 1399 | 1405 | 5000 | |
ThemeProvider | mount | 1211 | 1213 | 5000 | |
ThemeProvider | virtual-rerender | 648 | 645 | 5000 | |
ThemeProvider | virtual-rerender-with-unmount | 1919 | 1964 | 5000 | |
Toggle | mount | 811 | 794 | 5000 | |
buttonNative | mount | 131 | 135 | 5000 |
@@ -1,5 +1,5 @@ | |||
{ | |||
"extends": ["plugin:@fluentui/eslint-plugin/node"], | |||
"extends": ["plugin:@fluentui/eslint-plugin/node", "plugin:@fluentui/eslint-plugin/react"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like --legacy
isn't being used as it should. this is v8 package so it shouldn't use v9 lint rules at all.
This will need additional refactoring/checking across whole monorepo
@@ -73,8 +73,10 @@ export class CommandParser { | |||
} | |||
if (parsed.list) { | |||
const mods = getEnabledMods(console, getModsPaths); | |||
// eslint-disable-next-line no-console |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lint errors that appeared during config reshuffling
rules: { | ||
...configHelpers.getNamingConventionRule(true /* prefixWithI */), | ||
...configHelpers.getNamingConventionRule(true), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...configHelpers.getNamingConventionRule(true), | |
/* prefix interfaces with I */ | |
...configHelpers.getNamingConventionRule(true), |
might be worth keeping the comment ?
m
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
having second thoughts on the
WDYT? @ling1726 UPDATE:
|
63f14a6
to
6f2cf22
Compare
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
@@ -0,0 +1,357 @@ | |||
// @ts-check | |||
const configHelpers = require('../utils/configHelpers'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems that this should be the base
and then have core
and core-legacy
. At least that makes more sense to me.
Not blocking tho
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm you mean changing the names only or the inheritance chain as well ?
"comment": "feat: create core base for react/node/legacy lint configs", | ||
"packageName": "@fluentui/eslint-plugin", | ||
"email": "martinhochel@microsoft.com", | ||
"dependentChangeType": "none" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome that you set dependentChangeType
to none
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
learned my lesson from the best ;)
449ff00
to
cdce639
Compare
cdce639
to
7428691
Compare
…22128) * feat: create core base for react/node/legacy lint configs * style: fix lint errors * generate change files * create base and base-legacy configs * feat: make imports config public * undo changes * docs(eslint-plugin): update docs
Current Behavior
react lint config causes various issues when we wanna extend/change existing rules, as any kind of change in
react
config is applied everywhere else.our eslint plugin configuration inheritance hierarchy:
New Behavior
warning
overload on CIairbnb/base
our eslint plugin configuration inheritance hierarchy:
Lint speed comparison
yarn workspace @fluentui/react-text eslint --ext=.ts,.tsx src
Related Issue(s)
Fixes #22127