eslint-plugin-import icon indicating copy to clipboard operation
eslint-plugin-import copied to clipboard

"no-cycle" affecting type resolution between monorepo packages

Open toerndev opened this issue 3 years ago • 7 comments

Problem: I get additional errors from @typescript-eslint/require-await and @typescript-eslint/restrict-plus-operands after activating import/no-cycle.

Out of these "extra" errors, some seem legit:

  • a.b + c // a can actually be undefined here so "restrict-plus-operands" makes sense

...while in other cases, TS normally infers the number type but now suddenly requires more explicit/direct types for variables.

  • performance.now() isn't accepted as a number when no-cycle is on.
  • useState(0) can normally inferred to be a number, but now needs <number>.
  • maybeUndefined?.myNumber ?? 0 is no longer understood to be a number etc.

It's as if using a much older version of typescript. These errors also only appear when invoking eslint on multiple monorepo packages at once, with some files that import from the other package.

As for require-await, the following file with no imports only has an error when also running no-cycle:

export const sleep = async (delay: number) => {
 return new Promise<void>((resolve) => setTimeout(resolve, delay))
}
// "Async arrow function 'sleep' has no 'await' expression

Minimal setup to reproduce:

// .eslintrc.cjs
module.exports = {
 root: true,
 parser: '@typescript-eslint/parser',
 parserOptions: {
   allowAutomaticSingleRunInference: true,
   ecmaVersion: 2022,
   project: [
     './tsconfig.eslint.json',
     './common/tsconfig.json',
     './web/tsconfig.json',
   ],
   sourceType: 'module',
   tsconfigRootDir: __dirname,
 },
 plugins: ['@typescript-eslint', 'import'],
 extends: ['plugin:import/typescript'],
 rules: {
   '@typescript-eslint/require-await': 'error',
   '@typescript-eslint/restrict-plus-operands': 'error',
   'import/no-cycle': 'error',
 },
 settings: {
   react: { version: '17' },
   // required for differentiating between external/internal in rule 'import/order'
   'import/resolver': {
     typescript: {
       // The resolver needs to know where to find different node_modules,
       // otherwise it depends on which directory you are running eslint from.
       // The reason the other projects are in override settings is because there
       // seems to be a bug in the eslint-import-resolver-typescript package that
       // resolves all projects together (thereby importing from the wrong module)
       project: ['./tsconfig.eslint.json'],
     },
   },
 },
 overrides: [
   {
     files: ['common/**'],
     settings: {
       'import/resolver': {
         typescript: {
           project: ['./common/tsconfig.json'],
         },
       },
     },
   },
   {
     files: ['web/**'],
     env: {
       browser: true,
     },
     settings: {
       'import/resolver': {
         typescript: {
           project: ['./web/tsconfig.json'],
         },
       },
     },
   },
 ],
}

The tsconfig.json in each package:

{
  "extends": "../tsconfig.base.json",
  "compilerOptions": {
    "baseUrl": "src"
  },
  "include": ["src/**/*"],
}

Adding settings['import/resolver'].node.paths to match the baseUrl changes the number of errors a little bit.

toerndev avatar Jul 15 '22 09:07 toerndev

Setting aside that "require-await" is a harmful rule that should never be enabled, I can't conceive of why one rule being enabled would possibly impact a different one - especially in another plugin. eslint actively prevents rules from affecting each other.

Which version of the TS eslint resolver are you using?

ljharb avatar Jul 15 '22 17:07 ljharb

Right, those rules don't seem that great, I'll probably remove both. But they revealed that there are unknown & unexplained interactions in my linting which is pretty uncomfortable. :-)

Sorry forgot to include: typescript: 4.7.4 eslint: 8.19.0 eslint-plugin-import: 2.26.0 eslint-import-typescript-resolver: 3.2.5 @typescript-eslint/{parser,eslint-plugin}: 5.30.5

The reason that I added the TS resolver was to use import/order and differentiate between external packages resolved from node_modules, and internal packages that are absolute imports from ./src.

toerndev avatar Jul 16 '22 09:07 toerndev

Right, that all makes sense.

cc @bradzacher - any hints as to why one rule would impact another?

ljharb avatar Jul 16 '22 17:07 ljharb

I'd need a concrete reproduction case. @toerndev can you create an isolated GH repo with the minimal setup required to repro this issue?

bradzacher avatar Jul 16 '22 17:07 bradzacher

@ljharb separate and unrelated to the issue, just as an FYI. the ts-eslint version is smarter than the base rule as it allows returning promises (it's type-aware). It's poorly named because it builds on top of the base rule.

bradzacher avatar Jul 16 '22 18:07 bradzacher

@bradzacher that's helpful, but it's perfectly valid to have an async function that does not return a promise and does not use await, and it's massively harmful to even imply otherwise. The rule should never have existed in eslint (i totally understand why it therefore exists in TS-eslint, ofc)

ljharb avatar Jul 16 '22 18:07 ljharb

@bradzacher I peeled off as much as I could: toerndev/reproduce-eslint-issue

toerndev avatar Jul 19 '22 08:07 toerndev

Fix: https://github.com/typescript-eslint/typescript-eslint/pull/5893

bradzacher avatar Oct 26 '22 04:10 bradzacher

FYI - the fix has been released in v5.42.0 of the @typescript-eslint tooling.

So this issue can now be closed!

bradzacher avatar Oct 31 '22 21:10 bradzacher