import-js / eslint-plugin-import

ESLint plugin with rules that help validate proper imports.
MIT License
5.56k stars 1.57k forks source link

[no-extraneous-dependencies] improve validation of "import type" #2044

Open karlismelderis opened 3 years ago

karlismelderis commented 3 years ago

at the moment if I do this rule is not checking dependency: import type {TypeA} from 'ModuleA'

would be nice if such import would be checked against combination of dev and prod dependencies.

ljharb commented 3 years ago

That’s what tsc is for.

karlismelderis commented 3 years ago

well... I'm not expert in tsconfig

🙏 correct me if I'm wrong but package still can be available for different other reasons: ) installed globally ) installed at root of monorepo *) sub-dependency of another package

not sure if tsc would complain in these cases

where no-extraneous-dependencies explicitly guard you to put all dependencies in package.json

ljharb commented 3 years ago

Global things can't be required, but the other two could indeed apply. However, types are all transpiled out, so i'm not sure why it matters how the dependency is resolved?

karlismelderis commented 3 years ago

for example currently rule has configuration for dev dependencies. it's a useful feature for test files.

if you lift and shift project from monorepo it will stop working because modules that provide types are not found nor in prod nor dev dependencies.

So adding check for type imports would make each project more independent.

ljharb commented 3 years ago

What does "lift and shift" mean?

karlismelderis commented 3 years ago

if you decide to migrate project from repo to separate repo

if you decide to install dependencies for single project in repo if you decide to copy code of single project and run

there are enough scenarios where dependencies from e.g. root is not available.

ljharb commented 3 years ago

OK, fair point. I suppose import type deps should be listed in dev deps, and the rule should enforce that.

ljharb commented 3 years ago

The challenge is, how do you differentiate? Currently, you configure the rule per-directory, ie, "this dir, src, can have only deps" and "this dir, test, can have deps or dev deps". import type would be a dev dep in a place that normally (correctly) only allows deps.

karlismelderis commented 3 years ago

true. In our case it would be dev dependencies but in production code.

I assume that configuration to turn on/off dev dependencies is there to prevent compiled production code to reference dev dependency.

What if we extend case for type imports and just check if module is found in prod or dev dependencies? irrelevant of config? Project should be happy as long as such import is found in any of two dependencies.

ljharb commented 3 years ago

While I think that's the common case, we couldn't just impose that on everyone - you'd need to be able to opt in to it somehow.

karlismelderis commented 3 years ago

agree. some configuration option would be appropriate.

typeDependencies would be confusing.

noExternalTypeImports or noExtraneousTypeImports maybe?

iliubinskii commented 3 years ago

+1 for an option "alsoCheckTypeDependencies" or additional rule "noExtraneousTypeImports" with same options as "noExtraneousImports".

Typescript libraries are usually distributed as .js + .d.ts.

If your .ts file imports types from package A and consuming project does not have package A, it will fail to understand types defined in .d.ts.

So, even if you only import types from package A you may need to add it to dependencies or peer dependencies.

Sonarqube does not ignore type import by the way.

matthias-ccri commented 2 years ago

Here's a use case where this is needed — in a monorepo using lerna or rush, where packages depend on other packages in the monorepo. When you're building a package, its deps need to be built first. If there's an import (whether import type or not), it's an error if that package isn't listed as a dep or devDep, since it wouldn't get built before the current package.

So monorepo builds depend on complete deps lists, with no extraneous imports — not for runtime, but for build time. It would be nice if this rule checked the type-only imports too.

mgol commented 2 years ago

I agree with @ilyub: it's not correct to expect type imports to come from devDependencies. In some cases, your package (say, A) may export some types that embed types from a different package (B) and then you'd definitely want B installed when you have A in your dependencies or a part of the exported type will not resolve correctly.

Because of the above, I was actually surprised to learn import type is explicitly excluded by this rule; I expected it to be treated as all other imports by default.

That said, if there's any flag that makes type imports not excluded, that would also work.

ljharb commented 2 years ago

I’m fine to add it with an option in general; but @mgol’s comment means that we cant easily know whether a given type import comes from deps or dev deps - or perhaps even from a random “declare module” anywhere in the codebase.

I think that if you aren’t following the rule of importing types from devDep DT packages, there’s not much this rule can do - because there’s not much it can assume.