feat(eslint-plugin): add autofixable import rules #22050
Conversation
|
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 33e3181:
|
📊 Bundle size reportUnchanged fixtures
|
Hotell
changed the title
Hotell
changed the title
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 1074 | 1046 | 5000 | |
| Button | mount | 648 | 647 | 5000 | |
| FluentProvider | mount | 2099 | 2073 | 5000 | |
| FluentProviderWithTheme | mount | 325 | 288 | 10 | |
| FluentProviderWithTheme | virtual-rerender | 268 | 249 | 10 | |
| FluentProviderWithTheme | virtual-rerender-with-unmount | 310 | 345 | 10 | |
| MakeStyles | mount | 1798 | 1803 | 50000 |
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: fcbdedc99e36f562680ebd2b1a7d422dabd5078b (build) |
| { | ||
| ignorePatterns: [ | ||
| 'require(<.*?>)?\\(', | ||
| 'https?:\\/\\/', |
There was a problem hiding this comment.
most of these changes is just re-ordering/collocation of rules
| @@ -108,21 +88,15 @@ const config = { | |||
| 'no-empty': 'error', | |||
| 'no-eval': 'error', | |||
| 'no-new-wrappers': 'error', | |||
| // handles only member sort, rest is handled via import/order | |||
| 'sort-imports': ['warn', { ignoreDeclarationSort: true }], | |||
There was a problem hiding this comment.
new rule - handles sorting members within import statements
| @@ -273,15 +313,24 @@ const config = { | |||
| * @see https://github.com/import-js/eslint-plugin-import | |||
| */ | |||
| 'import/no-extraneous-dependencies': ['error', { devDependencies: false }], | |||
| 'import/no-duplicates': 'warn', | |||
There was a problem hiding this comment.
new rules
- handles sorting imports by path
- handles duplicate path imports
- enforces to have imports always at the top of the file ( default behaviour of ECMAscript anyways - imports are hoisted )
Perf Analysis (
|
| Scenario | Current PR Ticks | Baseline Ticks | Ratio | Regression Analysis |
|---|---|---|---|---|
| PortalMinimalPerf.default | 191 | 180 | 1.06:1 | analysis |
Perf tests with no regressions
| Scenario | Current PR Ticks | Baseline Ticks | Ratio |
|---|---|---|---|
| AccordionMinimalPerf.default | 183 | 164 | 1.12:1 |
| ButtonMinimalPerf.default | 213 | 193 | 1.1:1 |
| ListNestedPerf.default | 662 | 602 | 1.1:1 |
| DividerMinimalPerf.default | 421 | 390 | 1.08:1 |
| LayoutMinimalPerf.default | 422 | 391 | 1.08:1 |
| BoxMinimalPerf.default | 393 | 367 | 1.07:1 |
| FlexMinimalPerf.default | 333 | 311 | 1.07:1 |
| AttachmentMinimalPerf.default | 189 | 178 | 1.06:1 |
| TreeMinimalPerf.default | 935 | 884 | 1.06:1 |
| LabelMinimalPerf.default | 447 | 427 | 1.05:1 |
| TableMinimalPerf.default | 478 | 455 | 1.05:1 |
| TreeWith60ListItems.default | 208 | 199 | 1.05:1 |
| CardMinimalPerf.default | 664 | 637 | 1.04:1 |
| LoaderMinimalPerf.default | 766 | 734 | 1.04:1 |
| ReactionMinimalPerf.default | 442 | 424 | 1.04:1 |
| DatepickerMinimalPerf.default | 6134 | 5962 | 1.03:1 |
| ItemLayoutMinimalPerf.default | 1351 | 1309 | 1.03:1 |
| ListCommonPerf.default | 716 | 693 | 1.03:1 |
| VideoMinimalPerf.default | 746 | 725 | 1.03:1 |
| AnimationMinimalPerf.default | 602 | 591 | 1.02:1 |
| ChatWithPopoverPerf.default | 448 | 438 | 1.02:1 |
| FormMinimalPerf.default | 481 | 471 | 1.02:1 |
| HeaderMinimalPerf.default | 415 | 407 | 1.02:1 |
| ListWith60ListItems.default | 707 | 696 | 1.02:1 |
| ProviderMergeThemesPerf.default | 1846 | 1803 | 1.02:1 |
| RadioGroupMinimalPerf.default | 521 | 513 | 1.02:1 |
| AttachmentSlotsPerf.default | 1167 | 1157 | 1.01:1 |
| DialogMinimalPerf.default | 838 | 831 | 1.01:1 |
| DropdownManyItemsPerf.default | 775 | 765 | 1.01:1 |
| ListMinimalPerf.default | 576 | 571 | 1.01:1 |
| MenuMinimalPerf.default | 961 | 947 | 1.01:1 |
| PopupMinimalPerf.default | 686 | 676 | 1.01:1 |
| SkeletonMinimalPerf.default | 392 | 390 | 1.01:1 |
| SliderMinimalPerf.default | 1842 | 1830 | 1.01:1 |
| StatusMinimalPerf.default | 758 | 753 | 1.01:1 |
| TooltipMinimalPerf.default | 1128 | 1115 | 1.01:1 |
| AvatarMinimalPerf.default | 216 | 216 | 1:1 |
| ChatMinimalPerf.default | 835 | 839 | 1:1 |
| CheckboxMinimalPerf.default | 2871 | 2863 | 1:1 |
| DropdownMinimalPerf.default | 3245 | 3233 | 1:1 |
| EmbedMinimalPerf.default | 4556 | 4543 | 1:1 |
| GridMinimalPerf.default | 377 | 377 | 1:1 |
| ImageMinimalPerf.default | 423 | 421 | 1:1 |
| InputMinimalPerf.default | 1414 | 1412 | 1:1 |
| MenuButtonMinimalPerf.default | 1898 | 1903 | 1:1 |
| ProviderMinimalPerf.default | 1248 | 1251 | 1:1 |
| TextMinimalPerf.default | 402 | 400 | 1:1 |
| ToolbarMinimalPerf.default | 1069 | 1066 | 1:1 |
| ButtonOverridesMissPerf.default | 1808 | 1834 | 0.99:1 |
| ButtonSlotsPerf.default | 622 | 629 | 0.99:1 |
| CarouselMinimalPerf.default | 541 | 548 | 0.99:1 |
| ChatDuplicateMessagesPerf.default | 341 | 346 | 0.99:1 |
| RefMinimalPerf.default | 253 | 256 | 0.99:1 |
| SplitButtonMinimalPerf.default | 4788 | 4847 | 0.99:1 |
| TableManyItemsPerf.default | 2166 | 2180 | 0.99:1 |
| TextAreaMinimalPerf.default | 576 | 579 | 0.99:1 |
| IconMinimalPerf.default | 674 | 690 | 0.98:1 |
| CustomToolbarPrototype.default | 4360 | 4427 | 0.98:1 |
| HeaderSlotsPerf.default | 841 | 864 | 0.97:1 |
| SegmentMinimalPerf.default | 382 | 392 | 0.97:1 |
| AlertMinimalPerf.default | 302 | 314 | 0.96:1 |
| RosterPerf.default | 1242 | 1307 | 0.95:1 |
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| BaseButton | mount | 1050 | 1065 | 5000 | |
| Breadcrumb | mount | 2822 | 2829 | 1000 | |
| Checkbox | mount | 1667 | 1730 | 5000 | |
| CheckboxBase | mount | 1411 | 1471 | 5000 | |
| ChoiceGroup | mount | 5361 | 5276 | 5000 | |
| ComboBox | mount | 1104 | 1092 | 1000 | |
| CommandBar | mount | 11163 | 11108 | 1000 | |
| ContextualMenu | mount | 12136 | 12237 | 1000 | |
| DefaultButton | mount | 1274 | 1294 | 5000 | |
| DetailsRow | mount | 4223 | 4261 | 5000 | |
| DetailsRowFast | mount | 4164 | 4225 | 5000 | |
| DetailsRowNoStyles | mount | 4020 | 3944 | 5000 | |
| Dialog | mount | 2387 | 2335 | 1000 | |
| DocumentCardTitle | mount | 187 | 196 | 1000 | |
| Dropdown | mount | 3675 | 3714 | 5000 | |
| FocusTrapZone | mount | 1963 | 1972 | 5000 | |
| FocusZone | mount | 1956 | 1908 | 5000 | |
| IconButton | mount | 2018 | 2040 | 5000 | |
| Label | mount | 380 | 383 | 5000 | |
| Layer | mount | 3278 | 3322 | 5000 | |
| Link | mount | 534 | 532 | 5000 | |
| MenuButton | mount | 1688 | 1715 | 5000 | |
| MessageBar | mount | 2286 | 2301 | 5000 | |
| Nav | mount | 3666 | 3715 | 1000 | |
| OverflowSet | mount | 1182 | 1175 | 5000 | |
| Panel | mount | 2334 | 2344 | 1000 | |
| Persona | mount | 959 | 935 | 1000 | |
| Pivot | mount | 1589 | 1642 | 1000 | |
| PrimaryButton | mount | 1466 | 1450 | 5000 | |
| Rating | mount | 8792 | 8725 | 5000 | |
| SearchBox | mount | 1510 | 1557 | 5000 | |
| Shimmer | mount | 2799 | 2790 | 5000 | |
| Slider | mount | 2134 | 2148 | 5000 | |
| SpinButton | mount | 5535 | 5494 | 5000 | |
| Spinner | mount | 459 | 469 | 5000 | |
| SplitButton | mount | 3497 | 3536 | 5000 | |
| Stack | mount | 607 | 577 | 5000 | |
| StackWithIntrinsicChildren | mount | 2756 | 2757 | 5000 | |
| StackWithTextChildren | mount | 6125 | 6162 | 5000 | |
| SwatchColorPicker | mount | 12682 | 12720 | 5000 | |
| TagPicker | mount | 3054 | 3000 | 5000 | |
| TeachingBubble | mount | 102404 | 103616 | 5000 | |
| Text | mount | 492 | 477 | 5000 | |
| TextField | mount | 1601 | 1584 | 5000 | |
| ThemeProvider | mount | 1273 | 1286 | 5000 | |
| ThemeProvider | virtual-rerender | 668 | 689 | 5000 | |
| ThemeProvider | virtual-rerender-with-unmount | 2118 | 2087 | 5000 | |
| Toggle | mount | 897 | 897 | 5000 | |
| buttonNative | mount | 151 | 143 | 5000 |
| @@ -216,6 +143,93 @@ const config = { | |||
| 'operator-assignment': 'off', | |||
| 'prefer-destructuring': 'off', | |||
| 'prefer-template': 'off', | |||
| // airbnb or other config overrides (some temporary) | |||
| // TODO: determine which rules we want to enable, and make needed changes (separate PR) | |||
There was a problem hiding this comment.
Do we want to open a ticket for this right away?
There was a problem hiding this comment.
i dunno TBH. as stated in PR I didn't spent extra time to go through every rule rather colocating for better readability. Feel free to investigate further
There was a problem hiding this comment.
This comment is from back when we originally migrated to eslint. The idea back then was to extend the airbnb config (maybe because v0 did?) and eventually enable more rules as we thought it made sense, but we never got around to following up. Since we're disabling so many airbnb rules, it would probably make more sense to just directly enable the rules from there that we do want and remove that config extension (in a separate PR).
There was a problem hiding this comment.
this ^ 100%. I'd like us to remove airbnb completely (unmaintaned, loooots of opinionated rules that are not useful that much. I'll work on this as a follow up.
|
I would rename this PR since it seems the main part is adding new rules that weren't there before ? |
those rule have been there whole time just turned off, only new rule added is thoughts? |
Hotell
changed the title
Hotell
force-pushed
the
hotell/improve-eslint-import-rules
branch
from
596d38e to
cc5df72
Compare
change/@fluentui-eslint-plugin-e6e0684b-1ee7-4695-a151-9f716d78ab67.json
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I mentioned this in a comment but I'm still not convinced of the value of having rules which are warning level--unless we have a plan to fix them regularly, it just creates an ever-increasing amount of spam in build logs, and could be confusing for people when they see warnings that are completely unrelated to their change (enabling the rules adds hundreds (edit: thousands) of warnings to the logs).
| 'no-empty-character-class': 'off', | ||
|
|
||
| /** | ||
| * |
There was a problem hiding this comment.
I know this is a picky unimportant thing but all the blank lines starting the comments are bothering me, and IMO it would look nicer to get rid of them
There was a problem hiding this comment.
as you said, not relevant to this PR, we can address as follow up
| @@ -216,6 +143,93 @@ const config = { | |||
| 'operator-assignment': 'off', | |||
| 'prefer-destructuring': 'off', | |||
| 'prefer-template': 'off', | |||
| // airbnb or other config overrides (some temporary) | |||
| // TODO: determine which rules we want to enable, and make needed changes (separate PR) | |||
There was a problem hiding this comment.
This comment is from back when we originally migrated to eslint. The idea back then was to extend the airbnb config (maybe because v0 did?) and eventually enable more rules as we thought it made sense, but we never got around to following up. Since we're disabling so many airbnb rules, it would probably make more sense to just directly enable the rules from there that we do want and remove that config extension (in a separate PR).
| @@ -273,15 +313,24 @@ const config = { | |||
| * @see https://github.com/import-js/eslint-plugin-import | |||
| */ | |||
| 'import/no-extraneous-dependencies': ['error', { devDependencies: false }], | |||
| 'import/no-duplicates': 'warn', | |||
There was a problem hiding this comment.
Aren't the new rules going to cause a bunch of spam in build logs due to existing violations? (edit: it adds THOUSANDS of new warnings across all projects. 😱) And I still don't really see the point of even having rules if you can just ignore the warnings (especially since it creates a nuisance for others running lint because they see warnings from code they didn't even touch).
There was a problem hiding this comment.
Agree on warnings. They add a lot of noise and not much help. I'd prefer autofix or failing rules only. Otherwise, it probably isn't really a rule if it can't be autofixed or if we can't force the dev to fix it.
|
Have you looked at the overall performance of the lint tasks with and without these rules, and/or whether any of them show up in the top 10 slowest rules when running with timing? You can run with timing by either setting the Even if there's a significant perf penalty it could still be worth enabling some of the rules, but it would be good to know beforehand what we're signing up for. (In retrospect, I should have asked this on previous PRs adding lint rules too.) In particular I'm wondering if EDIT: I tried running with timing and |
|
I tried checking out this PR and running eslint with For example:
Those issues could be addressed with some further refinements to the config, but there will still be a large number of files with errors. I also noticed some minor issues with the fixers:
The following {
alphabetize: {
order: 'asc',
caseInsensitive: false,
},
groups: ['builtin', 'external', 'parent', 'sibling', 'index', 'type'],
pathGroups: [
{
pattern: '{react,react-dom}',
group: 'builtin',
position: 'before',
},
{
pattern: '{@fluentui,@griffel}/**',
group: 'external',
position: 'after',
},
],
pathGroupsExcludedImportTypes: ['{react,react-dom}', '{@fluentui,@griffel}/**'],
},All that to say...I agree it would be nice to have more orderly imports, and I'm okay with some version of this PR if we either update it or do a very fast follow-up to get rid of the thousands of warnings. But just introducing thousands of warnings into the build and leaving them there (until someone happens to modify the file again) is not acceptable. |
…8ab67.json Co-authored-by: Elizabeth Craig <[email protected]>
Thanks for bringing this up. I have. it's not the best but it's worth the cost from my experience. Any rule needing type information is far worse than this. Also rules from
That's fine from my experience. What is first or second doesn't improve our code, nor provides any benefit to consumers. Aim is to have consistency without need to nit-picking on peoples work based on "particular reviewer opinion"
This will need to be manually maintained if we add/change something else. is it worth it ? From my experience it is not. If majority of folks agree, then let's update it and maintain it. The argument of having "less warnings" at the moment will be mitigated soon as I mention at the end of this long comment.
I'd say this is out of topic for this PR and we can discuss this in bigger quorum. Also note I explicitly stated in PR description following: To provide brief context, based on my experience, having linting errors just increases anxiety for any contributor during pressing keystroke. lint errors are justified only for things that:
Anything else is mostly just unnecessary churn, thus having it as warning that is autofixable ( thus will be fixed anyways during commit ) is very pleasant DX/low effort from contributor to follow. Actionables:
Tooling constrains: Because eslint doesn't provide gradual rule application, to mitigate this we have only 2 options:
None of these approaches will move us to 1 goal - have all things consistently formatted To summarize: In general I wanna address existing issues, linked in this PR, that devs are facing on daily basis. 2 members of tooling team approved. Lemme know how do you wanna proceed @ecraig12345 based on additional options I provided. thanks |
|
@Hotell Would this PR be a good thing to cover in tech sync? It might be nice to get some team-wide input and awareness if this does introduce a large set of warnings to go fix. You fanned out the work items so nicely with the ts solution fixes - maybe that should happen again here? |
|
@Hotell If warnings are fixed as part of saving in vscode, could we run a nx script to run the es-lint formatter across all the files in a package and basically do a save of every file on behalf of the user? |
It's simpler than that--eslint has a |
|
I like the idea of more orderly imports, but here's my main concern with the current approach: unless we fix all the new "issues" immediately, this PR will introduce a substantial amount of confusion/extra mental load for all developers in our repo:
This is a substantial cost to impose on everyone, so I'd like to get agreement from the team about whether it's worth it. Both of the linked issues are ones you made, so it's not clear from that what the priority is from other people. (It's also not clear if the other reviewers realized just how many warnings this would cause.) For specific points:
Yes it is, as warnings
I edited my comment about this since I tested it and it turned out the perf concern from these rules isn't too bad, particularly compared to the deprecation rule. I never proposed re-implementing any import rules (I only did that for max-len based on specific data), just advocating understanding the cost vs. benefits before we enable new rules.
I agree the most important thing is automating it, but the default behavior of the rule itself acknowledges that there can be some value (even if it's minor) from "logical" grouping of imports: by default,
This shouldn't require maintenance. The only exceptions are for our primary component scope (I could go either way about whether to special-case griffel), and packages from our core library, so unless we rename our scope again it won't be an issue.
As outlined above, my concern is adding so many warnings will have the same effect, and worse.
At least to me, this is a bit different than the previous times we've applied that approach (which I'm still not a fan of to begin with, at least if it involves separate PRs for every package). This is forcing a pretty severe load of extra noise on everyone until the fixes are made.
If you don't want to do the changes all at once, we can track it with issues, same as we have before.
As mentioned above, it's not clear the level of priority this goal has for the team at large, and whether people consider it worth the tradeoff of having so many warnings. |
|
Made an issue for separate v8/v9 eslint configs - #22127. As discussed in the meeting, I'm more okay with this if it's enabled for v9 only, we just don't have a good way to do that right now since v8/v9 share a config. |
|
I second a lot of what @ecraig12345 is raising. Simple is better than complex. This applies to the rule system as well as the dev experience. We should guard the number of cycles a dev has to spend and the number of cycles the build has to spend. 🛑 Either fail or autofix the rule. Let's no require devs to think more than the bare minimum. They should focus on writing great code. All said, I don't think sorting imports is worth the cost to setup, maintain, and execute compared to the benefit it provides. We have a lot of high impact work that I'd love to see our energy going into instead. I will leave it at that and go with what the team decides. |
From your response it feels like you didn't read my reasoning, nor the PR description with live demos, can you help me understand if thats the case ?
warnings will disappear in time as people touch files ( everything is handled automatically ), which is also a good metric on progress. Goal should be to get rid of this.
It would be helpful to elaborate on what do you mean by "cost to setup", the cost of this setting up took me 20 seconds. Those rules are autofixable, so what are you referring to ?
this is the main goal of the tooling we are adding, as I mentioned during sync, focus on what consumer are getting not what is formatted how. I don't understand where do you see this approach as going against that mantra ?
as mentioned during sync and via referenced issues, This PR was created on real feedback from core framework devs |
|
based on discussion in this PR, it turns out we don't have common consensus. Following PR #22128, fixes inheritance issues with our current lint setup and adds new In the future, we might open this topic again, or as a followup if we see major adoption of autofixable import rules within v9 packages. |
Current Behavior
New Behavior
Fixable lint problems:
pre-commithookafter.mov
onSavein VSCodeafter-autofix.mov
Remarks
Note that this will trigger probably a lot of lint warnings on CI. we should create task so people can follow up and apply autofixes by themselves gradually.
Follow up/replacement of this PR
#22128
Related Issue(s)
Fixes #22049
Fixes #18594