New lint rule for sorting imports

[krawaller]

Description

The current linting of imports is done by the built-in sort-imports rule. It enforces alphabetical sorting, split between single and multiple imports. This is rather frustrating in practice since adding a property import to a pre-existing line might mean that you now have to move that line, and it is generally hard to figure out what the correct position for a line is.

This PR adds a new locally defined alternative to sort-imports which instead enforces having 3rd party imports before local imports. The hope is that this means bigger immediate benefit and less frustration. The rule also includes autofix to facilitate both initial adoption and subsequent workflow. The PR does not activate the new rule, so merging it is a noop.

Changes

• Adds the eslint-plugin-local-rules package, which is a simple pointer allowing us to define eslint rules locally in /eslint-local-rules (non-configurable, this name is hard-coded into the package)
• Adds definition for a sort-imports rule which separates 3rd party imports from local imports
• Adds the new rule to the .eslintrc file but turned off for now, as we want to do the initial fix run during a less PR-busy time

Notes to reviewer

• To understand the nuances of the rule, a good starting point is to look at the tests in ./eslint-local-rules/sort-imports.bdd.js.
• To do a test run of the rule with auto-fixer:
  1. In .eslintrc, switch local-rules/sort-imports from off to error (line 84)
  2. Also switch sort-imports from error to off (line 85)
  3. Run npx eslint . --fix in the terminal.
  4. Go through the 160+ changed files and check that the fixes are sound 🙈 😅
• There are some known limitations of the current implementation of the rule:
  1. It marks an error if you have non-import code intermingled inbetween import lines, but these are currently not corrected by the auto-fixer. I didn't deem it worth doing for now since the problem is only pre-existing in two files;
     * src/features/smartSearch/components/filters/Task/DisplayTask.tsx
     * src/zui/ZUITimeline/index.stories.tsx
  2. If there are several 3rd party imports in a row that need to be moved, the fixer will only move the first. This means that after the fixer has been run, you'll still have errors in the same file, and have to run the fixer again. This is caused by me cheating a bit to handle whitespace in the rule. This can be fixed by a refactor, but as the problem only pre-exists in a single file (src/pages/_app.tsx) I hoped I could get away with this cheat for now. When we do the initial adoption of the rule we have to run the fixer twice (thrice?), but after that it won't be an issue since it will correctly flag all errors.
  3. The rule right now treats built-in node packages as non-3rd-party imports, which might or might not be what we want. But right now there are just a couple of node files so I think this is ok:
     * integrationTesting/globalSetup.ts
     * integrationTesting/fixtures/next.ts
  • The rule doesn't add any special behaviours for the import of CSS files (import 'some/file.css';). Politically speaking it might make sense to force all of them together, or at least have them last in their respective group (3rd party and local). But right now the rule just checks that they are in the correct grouping.

[richardolsson]

Awesome work @krawaller! We will sit on this for a bit, until we're done merging a bunch of other PRs, so that we don't cause unnecessary conflicts in those. I look forward to being able to merge and activate this!

[WULCAN]

As we plan to implement this rule in Lyra first, I have reviewed this merge request in some detail. The code clearly communicates what it does and while there are some shortcuts, I believe I understand why they are taken and find them very reasonable.

If our project later uncovers edge cases that are not ideally handled by this rule, we can improve this rule then, rather than trying to predict which edge-cases will be possible and relevant.

I have not tested it on app.zetkin.org yet but I believe the you already did that. Before merging this, we should test the rule again, as new edge cases might have been added since you tested it.

Note that this rule only enforces that third-party dependencies are imported in a block separate from the other imports. It does no alphabetic sorting of declarations or members and it does no sorting according to type of import. As this rule is incompatible with eslint's sort-imports, enabling this rule will disable enforcement of all the other sorting rules.

[WULCAN]

I have reviewed 2.0.0 of the plugin local-rules, it is small and does what you expect it to. There might be some risk in running it without an eslint-local-rules in your working directory as it seems to keep searching for it up the hierarchy. There is also some risk of a future version of this plugin not doing what we expect but both risks are acceptable.

It seems like the more eslint-idiomatic way is to implement our rule in our own plugin instead and we can do that as a later improvement if we want to.

[WULCAN]

I have not reviewed this test file carefully.

[WULCAN]

I could not find documentation for the property category but looking at the built-in rules, "Possible Errors" seems like a too alarming category for something like this.

eslint's sort-imports is just categorized as "ECMAScript 6"

The categories I could find in eslint's rules are

• Best Practices
• ECMAScript 6
• Node.js and CommonJS
• Possible Errors
• Strict Mode
• Stylistic Issues
• Variables

The documentation also shows a different categorization on https://eslint.org/docs/v8.x/rules/

• Possible Problems
• Suggestions
• Layout & Formatting
• Deprecated
• Removed

Layout & Formatting only has two rules and they both have "Stylistic Issues" as category.

Note also that https://eslint.org/docs/v8.x/extend/custom-rules has multiple examples and never mentions any category property.

[richardolsson]

We are coming to a point where it should be possible to merge and activate this. I would just really love it if @voxpelli could have a look with his eslint eyes. 🙂

[voxpelli]

Have you looked into the existing import sorts that exists for ESLint?

At Socket I eg. set up this:

    '@typescript-eslint/consistent-type-imports': [
      'error',
      { 'prefer': 'type-imports', 'disallowTypeAnnotations': true }
    ],
    'import/order': [
      'warn',
      {
        'groups': ['builtin', 'external', ['internal', 'parent', 'sibling', 'index'], 'type'],
        'pathGroups': [
          {
            'pattern': 'react',
            'group': 'builtin',
            'position': 'before'
          }
        ],
        'pathGroupsExcludedImportTypes': ['react'],
        'newlines-between': 'always',
        'alphabetize': {
          'order': 'asc'
        }
      }
    ]

Are you sure a custom rule is needed over something like the above import/order config?

(See also the other relevant parts of that config: https://github.com/SocketDev/eslint-config/blob/82ad884980c1825dff7eefcab5a786f3cf325962/index.js#L20-L39)

[richardolsson]

Are you sure a custom rule is needed over something like the above import/order config?

(See also the other relevant parts of that config: https://github.com/SocketDev/eslint-config/blob/82ad884980c1825dff7eefcab5a786f3cf325962/index.js#L20-L39)

That's exactly why I wanted you to take a look at this. 😊

I have tried it now and I think I was able to achieve the desired effect using this config:

        'import/order': [
          'error',
          {
            alphabetize: {
              order: 'ignore',
              orderImportKind: 'ignore',
            },
            groups: [
              ['external', 'builtin'],
            ],
            'newlines-between': 'always',
          },
        ],

As such, I think this is actually preferred over the solution in this PR. I'm very sorry to come to this conclusion because it was very impressive work done by @krawaller. 🙌 I hope you feel you learned something about eslint along the way!

[richardolsson]

Closing this in favor of #2065.