Proposal for new rule - consistent style for type-only imports

[nojvek]

Proposal

{
  "rules": {
    "@typescript-eslint/prefer-import-type": ["error"]
  }
}

// your repro code case
import {FooBar} from 'foo-bar';
const foo: FooBar = {foo: 1, bar: 2};

Expected Result
FooBar is only being used as a type, import as 'import type {FooBar}'

Actual Result

Additional Info

Since typescript now supports import type {X} for type only imports that don't get emitted in js, nor do get counted as cyclic imports, it would be nice to have typescript-eslint automatically flag and fix import {T} to import type {T}

If you're willing to accept the suggestion, I'm happy to create a PR and land this

[bradzacher]

This is technically possible to build right now, but it will be much easier once we release the scope analyser: #1939

[felixfbecker]

I would love to have such a rule, with per-package configuration. For some packages I only want to allow type-only imports to ensure the package never ends up in the runtime bundle.

[felixfbecker]

This is a duplicate though of #1985, right?

[bradzacher]

they're not quite the same.

#1985 is about building a rule which requires that all types are imported via import type.
This issue is about building a rule which requires that things that are only used in a type position are imported via import type.

They could probably be merged together, as technically this request will cover #1985 (but #1985 won't cover this request).

For example, this case would error in this propsed rule, but not in #1985, because Component is a value.

import { Component } from 'react';
// error: Component is only used as a type, so it should be imported using a type only import
type Test = Component;

[bradzacher]

I'll merge the two into this one.

requirements for rule:

• option to specify preference (prefer type imports or no type imports) - default "type imports".
• option to disable imports in type annotations (const x: import('foo')) - default true.

[rfermann]

#1985 is about building a rule which requires that all types are imported via import type.

That wasn't my exact intention for proposing this rule. The intention was to enforce one style of type imports. This enforced style could be a type import, but it could also be an import in a type annotation or just a regular import.

• option to specify preference (prefer type imports or no type imports) - default "type imports".
• option to disable imports in type annotations (const x: import('foo')) - default true.

Would these rules also allow to prefer imports in type annotations instead of using regular imports or type imports?

[bradzacher]

Would these rules also allow to prefer imports in type annotations instead of using regular imports or type imports?

It could.
Though I've never seen anyone use that style in TS code, only in JSDoc types in JS code.

Within TS code that style is ridiculously verbose and the repetition required very much goes against every programming principle I've ever learnt.
TBH I wouldn't really want to support that option.

[adidahiya]

This rule would be useful to me, particularly in conjunction with eslint-plugin-import's no-cycle rule. no-cycle works well, but is not smart enough to inspect imported symbols to distinguish between types and values. It flags type-only imports as cyclic dependencies, but I don't really care about those, since they have no runtime impact, while other cyclic dependencies can cause bugs. If we had a rule to enforce the import type syntax where appropriate, it would make managing cyclic dependencies easier.

[bradzacher]

I actually think we could accomplish this right now, but I know our current scope analyser is wildly inconsistent and inaccurate.

Once #2039 is merged, that will no longer be the case, but That really needs to wait for the 4.0.0 release as it's a pretty big change and is kinda breaking as it will report a lot more errors for users, and likely will require some reconfiguring for them..

Note that due to the direction of the ecosystem, we're going to have to make a few breaking changes to our AST very soon (read: likely within a month - exact timing depends on eslint/eslint#13416).

If someone wants to start building this on top of #2039, then we would be in a good state to ensure it is released as soon as 4.0.0 is ready.

[bradzacher]

This has been released to NPM as part of the prerelease version for v4.

Try out the rc-v4 tag on NPM.

Please raise any bugs you find as a new issue so we can track and fix them before the full release!
We're working on finalising all the work for the v4 release so eta for full release is uncertain.

https://github.com/typescript-eslint/typescript-eslint/milestone/5
https://github.com/typescript-eslint/typescript-eslint/releases/tag/v4.0.0-rc

[Blasz]

Hi @bradzacher, perhaps I'm missing something but what is the difference between this proposal and #1786 which was closed for duplicating an existing TS error?

The reason I ask is I'm considering proposing another TS 3.8 type syntax rule that also duplicates an existing TS error (error TS1205: Re-exporting a type when the '--isolatedModules' flag is provided requires using 'export type').

[bradzacher]

A few reasons I can see:

1. framing of the problem. The other issue framed the request in the context of a TS error. The better solution for that was to add a fixer to the language service.

2. timing. I was almost finished the scope analyser when this was raised (which is a critical component to enabling this lint rule). Because of the scope analyser, the work required to build this lint rule fell to mostly 0. The rule logic itself is only a few hundred lines (the fixer is the huge ugly bit).

3. memory. We get ~100 issues every month. Some I remember and some I don't. Add to that trying to remember every TS error. I don't always remember things - I completely forgot about that issue and (more importantly) the compiler option.

In hindsight - for this case I don't mind having this rule.
TS compiler options stop the build. For production builds this is great, but during dev this sucks.
I like the idea of this rule for the same reason I like the no-unused-vars rule over its compiler counterparts - it won't get in the way of development.
For example if you comment out the last value usage of an import, your build will block until you convert the import. This is frustrating and slow.

For your request, I might lean toward this being implemented in the TS language server for a few reasons:

• we can't detect isolatedModules mode without type-aware linting
  • does this matter though? Probably not. You'd configure the rule irrespective of the compiler option.
• we can't detect if an imported thing is a type for all cases without type-aware linting.
  • we can only reliably detect if a local thing is a type.
• not converting an export could actually break the build, as the transpiler might emit an incorrect export.

[Blasz]

Fair call!

I agree that it would make sense for typescript to handle this since it's an actual typescript error. The downside is that, as far as I'm aware, tsc/ts language server doesn't have the nice auto fixing functionality that eslint has where you can run it in bulk across a whole project. We're currently switching the isolatedModules flag on and have a ton of violations that I was hoping to fix somehow, perhaps I can write a short script that calls the TS language server directly to replicate the bulk fixing functionality of eslint.

[bradzacher]

For one-off migrations I would recommend a proper codemod tool like jscodeshift.
You could probably do something like run tsc, dump the errors, and use those to help drive a codemod.
At FB we do stuff like this all the time to codemod ignore/disable comments across the codebase.