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

[Fix] no-unresolved: ignore type-only imports #2220

Merged
merged 2 commits into from Sep 18, 2021
Merged

Conversation

@jablko
Copy link
Contributor

@jablko jablko commented Sep 7, 2021

I'm getting the following errors when linting TypeScript, without installing eslint-import-resolver-typescript:

  1:24  error  Unable to resolve path to module 'mdast'  import/no-unresolved
  3:24  error  Unable to resolve path to module 'unist'  import/no-unresolved

Can import/no-unresolved: off be added to the plugin:import/typescript preset, by the same logic as in #1726 (TypeScript compilation already confirms that imports are resolved)?

config/typescript.js Show resolved Hide resolved
Loading
@ljharb
Copy link
Collaborator

@ljharb ljharb commented Sep 7, 2021

Does typescript actually confirm resolution for untyped imports?

Loading

@jablko
Copy link
Contributor Author

@jablko jablko commented Sep 7, 2021

Does typescript actually confirm resolution for untyped imports?

Yes (I'm pretty sure). If I rm --recursive node_modules/@types and tsc --build, I get:

index.ts:1:24 - error TS2307: Cannot find module 'mdast' or its corresponding type declarations.

1 import * as mdast from "mdast";
                         ~~~~~~~

index.ts:3:24 - error TS2307: Cannot find module 'unist' or its corresponding type declarations.

3 import * as unist from "unist";
                         ~~~~~~~

I can't think of a TypeScript setup where that wouldn't be the case ...

Loading

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Sep 7, 2021

Those are types packages. I’m talking about untyped commonjs packages, that the lint rule enforces resolution on.

Loading

@codecov
Copy link

@codecov codecov bot commented Sep 7, 2021

Codecov Report

Merging #2220 (4ed7867) into main (9ccdcb7) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2220      +/-   ##
==========================================
+ Coverage   84.03%   84.04%   +0.01%     
==========================================
  Files          96       96              
  Lines        3032     3034       +2     
  Branches      899      900       +1     
==========================================
+ Hits         2548     2550       +2     
  Misses        484      484              
Impacted Files Coverage Δ
config/typescript.js 100.00% <100.00%> (ø)
src/rules/no-unresolved.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ccdcb7...4ed7867. Read the comment docs.

Loading

@jablko
Copy link
Contributor Author

@jablko jablko commented Sep 7, 2021

Those are types packages. I’m talking about untyped commonjs packages, that the lint rule enforces resolution on.

Sorry, good point: TypeScript doesn't complain that node_modules/mdast is missing (unless node_modules/@types/mdast is also missing). If eslint-import-resolver-typescript is installed, neither does import/no-unresolved.

That works for me: There's no mdast untyped package. But if I add export function notImplemented(): void; -> node_modules/@types/mdast/index.d.ts and call mdast.notImplemented() from my index.ts, I get Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'mdast' at runtime (sad but predictable).

As far as I know, eslint-import-resolver-typescript can get the kind of import (type-only or value) from the TypeScript parser -- based on usage, regardless of the import type syntax? Or maybe it merely parses without analysis ...

Ideally eslint-import-resolver-typescript would only consider node_modules/@types to satisfy type-only imports, and import/no-unresolved would error on value imports. Which implies import/no-unresolved: error, as today.

∴ my mistake, thanks for spotting! I'll close this and maybe investigate eslint-import-resolver-typescript + import kinds.

Loading

@jablko jablko closed this Sep 7, 2021
@jablko jablko changed the title [Patch] TypeScript config: disable import/no-unresolved [Fix] no-unresolved: ignore type-only imports Sep 11, 2021
@jablko
Copy link
Contributor Author

@jablko jablko commented Sep 11, 2021

Reopening this after revising the title because it carries on from the previous discussion: Although TypeScript doesn't confirm that value imports can be resolved, it does confirm that type-only imports can.

∴ instead of disabling import/no-unresolved in the plugin:import/typescript preset, like in #1726, disable it for type-only imports like in #1057.

Also, import/no-unresolved currently misses imports that can't be resolved if it finds a .d.ts with the same name. For example, if you import * as name from "module-name" and import/no-unresolved should error because module-name can't be resolved, except that a module-name.d.ts exists, then it won't in fact report an error. Instead you'll get Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'module-name' at runtime.

∴ remove .d.ts from the plugin:import/typescript preset.

The result is that import/no-unresolved will catch imports that can't be resolved to a .ts/.tsx/.js/.jsx implementation, and TypeScript will catch unresolvable type-only imports (which import/no-unresolved will now ignore).

Details

Removing .d.ts should only affect you if you have an import with a .d.ts but no .ts/.tsx/.js/.jsx implementation -- in which case you probably want the error? If you don't want the error because you're only using types and eliding the import at runtime, your options are: make it an explicit type-only import, or restore .d.ts to extensions in your own ESLint config?

In addition to explicit type-only imports, I investigated ignoring implicit type-only imports: when you import * as name from "module-name", but never use name in a value context (only types). What I found was:

  1. If a .ts/.tsx/.js/.jsx can't be resolved, import/no-unresolved should probably still error, because depending on the importsNotUsedAsValues option you'll get Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'module-name' at runtime:
    • error: error at compile time
    • preserve: error at runtime
    • remove: no error (this just happens to be the default)
  2. We couldn't ignore implicit type-only imports if we wanted to because it's done with canConvertImportDeclarationToTypeOnly() which is an internal API?

So I think the choice is between:

  1. Keep .d.ts in the preset:
    • miss value imports without a resolvable .ts/.tsx/.js/.jsx implementation
    • miss implicit type-only imports that error if importsNotUsedAsValues is preserve
  2. Remove .d.ts:
    • error on value imports without a resolvable implementation (good, right?)
    • error on implicit type-only imports that otherwise only error if importsNotUsedAsValues is preserve (must make them explicit or restore .d.ts yourself)

Note that case 1 (keep .d.ts) will still error on implicit type-only imports from @types packages, because only eslint-import-resolver-typescript understands those:

  • This PR fixes import/no-unresolved on explicit type-only imports, in both cases (so now you can silence these import/no-unresolved @types errors by making them explicit)
  • You still might want eslint-import-resolver-typescript to find extraneous @types dependencies (no-extraneous-dependencies)

Loading

src/rules/no-unresolved.js Outdated Show resolved Hide resolved
Loading
@jablko jablko force-pushed the patch-1 branch 4 times, most recently from 3a13ee5 to caec287 Sep 17, 2021
@jablko jablko requested a review from ljharb Sep 17, 2021
@jablko jablko force-pushed the patch-1 branch 2 times, most recently from cc7de4a to caec287 Sep 17, 2021
ljharb
ljharb approved these changes Sep 18, 2021
Copy link
Collaborator

@ljharb ljharb left a comment

This seems reasonable to me. .d.ts is of course redundant when we have .ts; and no-unresolved shouldn't bother checking type imports since tsc/flow does that for us. Thanks!

Loading

@ljharb ljharb merged commit 4ed7867 into import-js:main Sep 18, 2021
183 of 184 checks passed
Loading
@jablko
Copy link
Contributor Author

@jablko jablko commented Sep 19, 2021

Thank you!

Loading

@Hypnosphi
Copy link
Contributor

@Hypnosphi Hypnosphi commented Oct 14, 2021

It still gives me an import/extensions error for explicit type-only imports from my local d.ts files. I'll set up a repro in a moment

Loading

@Hypnosphi
Copy link
Contributor

@Hypnosphi Hypnosphi commented Oct 14, 2021

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants