Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"no-cycle" affecting type resolution between monorepo packages #2496

Closed
toerndev opened this issue Jul 15, 2022 · 9 comments · Fixed by typescript-eslint/typescript-eslint#5893

Comments

@toerndev
Copy link

toerndev commented Jul 15, 2022

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 toerndev changed the title "no-cycle" affecting type resolving between monorepo packages "no-cycle" affecting type resolution between monorepo packages Jul 15, 2022
@ljharb
Copy link
Member

ljharb commented Jul 15, 2022

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?

@toerndev
Copy link
Author

toerndev commented Jul 16, 2022

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.

@ljharb
Copy link
Member

ljharb commented Jul 16, 2022

Right, that all makes sense.

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

@bradzacher
Copy link
Contributor

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
Copy link
Contributor

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

@ljharb
Copy link
Member

ljharb commented Jul 16, 2022

@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)

@toerndev
Copy link
Author

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

@bradzacher
Copy link
Contributor

Fix: typescript-eslint/typescript-eslint#5893

@bradzacher
Copy link
Contributor

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

So this issue can now be closed!

@ljharb ljharb closed this as completed Nov 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants