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 report

Perf Analysis (@fluentui/react-components)

No significant results to display.

Asset size changes

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

Baseline commit: fcbdedc99e36f562680ebd2b1a7d422dabd5078b (build)

Perf Analysis (@fluentui/react-northstar)

⚠️ 1 potential perf regressions detected

Potential regressions comparing to master

Perf Analysis (@fluentui/react)

No significant results to display.

I would rename this PR since it seems the main part is adding new rules that weren't there before ?

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 sort-imports.

thoughts?

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 TIMING environment variable, or passing --timing to just-based eslint tasks. https://eslint.org/docs/developer-guide/working-with-rules#per-rule-performance

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 import/no-duplicates was a bit expensive, given that I previously had it disabled in favor of the simpler/"mostly good enough" no-duplicate-imports.

EDIT: I tried running with timing and import/order is sometimes near the top, but deprecation/deprecation is orders of magnitude worse (which is unfortunate, because at least IMO that one does add value).

I tried checking out this PR and running eslint with --fix locally...it's modified over 5000 files (meaning there are probably at least 2x that many warnings) including a lot of changes to import ordering that at least to me don't seem like an improvement.

For example:

• Most of our tsx files have React import as the very first, but now it's moved to/near the end of the list, due to @fluentui imports being interpreted as "external." (This and adding spaces between all groups account for at least half of the changed files.)
• Right now it's considering @fluentui, @griffel, and @storybook as equivalent externals and lumps them together in a group. @fluentui (and maybe @griffel) ought to be in a slightly different category IMO.

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 fixer doesn't properly move comments, including // @ts-ignore (which we should almost always avoid anyway...but if we do have them, they need to stay with the proper line)
• The fixers require multiple passes to get to 0 errors

The following import/order config a bit more closely matches some of the patterns we have now. It's down to "only" 2000+ files (or likely at least 2x that many warnings)...which is still a lot of files to modify, and definitely too many warnings to introduce into everyone's builds if we don't fix them upfront. (I actually like the idea of newlines between groups, but not if it causes an excessive number of warnings that we don't plan to fix upfront.) Here's the config reference--I'm not 100% sure what pathGroupsExcludedImportTypes means or valid values based on the documentation, but the way I used it below seems to be necessary to make the special cases be ordered how I expected.

      {
        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.

@ecraig12345

I'm wondering if import/no-duplicates was a bit expensive

I don't follow, this PR is not enabling that rule UPDATE: sorry i miss-read ( it was supposed to be deprecation/deprecation) sorry about that.

Have you looked at the overall performance of the lint tasks with and without these rules

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 import plugin are problematic in general in terms of performance. Is there valid business case to re-implement those rules on our own and maintain those ? I don't think so ATM.

Most of our tsx files have React import as the very first, but now it's moved to/near the end of the list, due to @FluentUI imports being interpreted as "external." (This and adding spaces between all groups account for at least half of the changed files.)

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"

pathGroups: [ {

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.

And I still don't really see the point of even having rules if you can just ignore the warnings

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:

• may cause security vulnerability
• may cause runtime errors
• may cause serious performance hits

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:

• the idea is that as we did multiple times already, every team will apply those fixes in their components

Tooling constrains:

Because eslint doesn't provide gradual rule application, to mitigate this we have only 2 options:

• add these rule package by package + apply fixes
  • you cannot measure/track this approach in any way (current approach gives you explicit data)
• enable quite mode, so warning are not logged on CI
  • again you cannot measure/track this approach

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?

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 --fix option, so you can either run yarn lint --fix in an individual package, or yarn lage lint --fix for the whole repo. (In either case you may need two passes to ensure everything is fixed.)

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:

• It becomes much harder to find actual lint errors or other build errors, in either a PR or local build, due to the sea of lint warnings.
• It will create confusion/demand extra mental energy from developers (especially outside the core team who don't know about this change), possibly at multiple points in the process:
  • Anyone editing a file will see a bunch of things that appear to be unresolved issues sitting there, which can require extra mental energy to evaluate whether they need to address or not.
    • The warnings will disappear on save or commit, but then there's confusion about "why did my commit touch all these unrelated lines."
  • It's likely that developers will be confused when their PR or local build is suddenly be filled with warnings, and whether they need to do anything about that.

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:

I'm wondering if import/no-duplicates was a bit expensive

I don't follow, this PR is not enabling that rule

Yes it is, as warnings
https://github.com/microsoft/fluentui/pull/22050/files#diff-76039af2f09c079049956d7b4b45efee6d01993677e1680519f5863fae6f0919R316

Have you looked at the overall performance of the lint tasks with and without these rules

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 import plugin are problematic in general in terms of performance. Is there valid business case to re-implement those rules on our own and maintain those ? I don't think so ATM.

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.

Most of our tsx files have React import as the very first, but now it's moved to/near the end of the list, due to @fluentui imports being interpreted as "external." (This and adding spaces between all groups account for at least half of the changed files.)

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"

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, ["builtin", "external", "parent", "sibling", "index"]. The order I proposed is a natural extension of the default order for our context, and aligns with conventions that are already widely used within our repo. (I could go either way on whether to have separate ordering for type imports.)

pathGroups: [ {

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.

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.

To provide brief context, based on my experience, having linting errors just increases anxiety for any contributor during pressing keystroke.

As outlined above, my concern is adding so many warnings will have the same effect, and worse.

Actionables:

• the idea is that as we did multiple times already, every team will apply those fixes in their components

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.

Tooling constrains:

Because eslint doesn't provide gradual rule application, to mitigate this we have only 2 options:

• add these rule package by package + apply fixes

  • you cannot measure/track this approach in any way (current approach gives you explicit data)

If you don't want to do the changes all at once, we can track it with issues, same as we have before.

• enable quite mode, so warning are not logged on CI

  • again you cannot measure/track this approach

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.

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.

⚠️ Let's not flood warnings as it removes the usefulness of the rule. People ignore things that flood since it is overwhelming.

🛑 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.

@levithomason

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.

⚠️ Let's not flood warnings as it removes the usefulness of the rule. People ignore things that flood since it is overwhelming.

🛑 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 ?

Let's not flood warnings as it removes the usefulness of the rule.

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.

All said, I don't think sorting imports is worth the cost to setup, maintain, and execute compared to the benefit it provides.

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 ?

They should focus on writing great code.

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 ?

I will leave it at that and go with what the team decides.

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 imports config which particular teams can opt into, to leverage autofixable imports, as they see fit, to address issues linked in this PR that triggered these changes in first place.

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.