Enforce type imports and import sort in ESLint

[dwrth]

Description

This PR introduces multiple new ESLint rules and changes to the typescript config. It also fixes/refactors effected parts in the codebase.

ESLint:

• consistent-type-imports
• recommended and typescript rules from the eslint-plugin-import
• import/order from the eslint-plugin-import package

The reason for choosing import/order over the builtin sort-imports rule are the autofix capabilities which are better in this external package.

TSConfig:

• verbatimModuleSyntax

Validation

Error highlighting for wrongly sorted imports:

With available autofix:

Error highlighting for incorrect type imports:

With available autofix:

Related Issues

Resolves #1118

Check List

• I confirm that my contribution is made under the terms of the Apache 2.0
  license.
• I have run pnpm checks to ensure code compiles and meets standards.
• I have run pnpm test to check if all tests are passing.
• I have covered new added functionality with unit tests if necessary.
• I have added an entry in the Changelog.md.

[kmcginnes]

This is great! Nice work @dwrth.

My two initial thoughts are:

• Can we align more with the default/recommended rules from the import plugin?
• Can we enable "verbatimModuleSyntax": true in the global tsconfig for the whole app?

[dwrth]

Not quite ready for the next review here but I got all the recommended configs working now without any rule tweaking!

Still trying to figure out an issue I’m having with the "verbatimModuleSyntax" option

[dwrth]

@kmcginnes Apart from the merge conflicts (which I was not able to resolve due to the new import sorting rules) I managed to implement all requested changes now.

I have adjusted the description of the PR accordingly!

[kmcginnes]

@dwrth Nice work! I'll be on vacation for the next week, so you won't hear much from me until I get back.

When I get back I'll check out the rule changes and verify the auto fix changes look decent. Then we can discard all of your existing auto fixes, rebase the branch on the latest main, then re-run the auto-fixes. That should leave us in a clean state to merge.

Sound like a plan?

[dwrth]

@kmcginnes Sounds great!

Enjoy your time off!

[kmcginnes]

ok, I've had a chance to look things over and read up a bit on how all this works. I think I have enough information to formulate an opinion.

Let's split the efforts in to two separate issues/PRs. The first PR would tackle the import type and verbatimModuleSyntax. The second PR (after the first one merges) would deal with sorting.

Splitting them up makes sense to me because the verbatimModuleSyntax change is relatively straightforward and can be reviewed pretty easily. The sorting changes are a bit more opinionated and reviewing those changes will be time consuming.

Type Imports

This should reduce down to just a few changes:

• Add verbatimModuleSyntax to tsconfig.base.json which will apply to all tsconfigs
  • Remove verbatimModuleSyntax from tsconfig.root.json, which doesn't really do anything
  • Remove verbatimModuleSyntax from packages/graph-explorer-proxy-server/tsconfig.json since it is now in the base config
• Use @typescript-eslint/consistent-type-imports auto fix to fix everything automatically

  "@typescript-eslint/consistent-type-imports": [
    "error",
    {
      fixStyle: "inline-type-imports",
    },
  ],

  • Commit automatically fixed files
  • Remove @typescript-eslint/consistent-type-imports because it has some conflicting behavior

This sets the repo up to use type imports properly and any new code will produce an error if it doesn't use type imports when it should.

Sort Imports

I'll create another GitHub issue for the sorting. I'll have to look a bit closer at eslint-plugin-import before I feel comfortable bringing it in. I think you are right, that it is the right choice over other options. But it is quite extensive and I'll need to think through the sorting preferences it makes. I also don't love that we'd need an additional TypeScript resolver system on top of the typescript-eslint resolver. These tend to slow linting to a crawl. Hopefully this isn't the case, but I'll need to do some testing.

[dwrth]

I did not know that a force push would just auto-close this PR but maybe it's also for the better continuing this on a fresh branch with appropriate name :D

#1231

[kmcginnes]

@dwrth Interesting. I had not seen that behavior from GitHub before. I force push all the time.

Oh well. I agree, it might be better to start fresh.