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

import/no-unresolved errors for .d.ts files since 2.25 #2267

Closed
mrdrogdrog opened this issue Oct 16, 2021 · 29 comments
Closed

import/no-unresolved errors for .d.ts files since 2.25 #2267

mrdrogdrog opened this issue Oct 16, 2021 · 29 comments

Comments

@mrdrogdrog
Copy link

mrdrogdrog commented Oct 16, 2021

Environment

Node: 16.11.1
ESLint: 7.32.0
Operating System: Linux

What happened?

We tried to update eslint-plugin-import in https://github.com/hedgedoc/react-client from 2.24.2 to 2.25.2 but eslint fails now. It can't resolve paths to .d.ts files anymore.

What did I expect?

No breaking changes in behaviour since it isn't a major release.

Additional Information

Feel free to take a look at our CI log.
https://github.com/hedgedoc/react-client/runs/3915016791?check_suite_focus=true

Thanks for your help

@andersk
Copy link
Contributor

andersk commented Oct 17, 2021

This was introduced by 4d15e26 (#2220). Cc @jabiko.

Minimal reproduction:

// foo.ts
import { bar } from "./bar";
console.log(bar);
// bar.d.ts
export const bar: string;
// .eslintrc.json
{
  "plugins": ["@typescript-eslint", "import"],
  "parser": "@typescript-eslint/parser",
  "extends": "plugin:import/typescript",
  "rules": {
    "import/no-unresolved": "error"
  }
}
$ npm i -D @typescript-eslint/eslint-plugin@5.0.0 @typescript-eslint/parser@5.0.0 eslint@7.32.0 eslint-plugin-import@2.25.2 typescript@4.4.4
$ npx eslint foo.ts
 
/home/anders/zulip/test/eslint-import-bug/foo.ts
  2:21  error  Unable to resolve path to module './bar'  import/no-unresolved

✖ 1 problem (1 error, 0 warnings)

@ljharb
Copy link
Member

ljharb commented Oct 17, 2021

@andersk that looks like a bug fix to me - if there’s no actual bar file there, then what are you importing? d.ts files are for providing types, not for pretending there’s a real runtime value there.

In other words, there needs to also be a bar.js file there next to the bar.d.ts file.

@mrdrogdrog
Copy link
Author

In a pure typescript environment it's normal to use .d.ts files that just provide types or interfaces but no functions. E.g. if you want to share types between multiple ts files.

@mrdrogdrog
Copy link
Author

Even if this is a bug fix, it's still a breaking change in behaviour and shouldn't be present in a patch release.

@mrdrogdrog
Copy link
Author

mrdrogdrog commented Oct 17, 2021

Just tested: One way to make this work without renaming all .d.ts files to .ts is to replace the import with a type import.

import { bar } from "./bar";
becomes
import type { bar } from "./bar";

A bit annoying if you have many many .d.ts files like
image

@mrdrogdrog
Copy link
Author

Adding

"@typescript-eslint/consistent-type-imports": [
        "error",
        {
          "prefer": "type-imports",
          "disallowTypeAnnotations": false
        }
      ]

to my eslint config helped a lot, because then eslint was able to autofix this.

But still: This breaking changes in patch releases are bad :/

@ljharb
Copy link
Member

ljharb commented Oct 17, 2021

@mrdrogdrog it's not normal to have a .d.ts file and no corresponding .js file to be imported.

Yes, breaking changes in patch releases are bad, but every single bug fix is a breaking change if someone's relying on the bug.

I see what you mean tho - in this case, you're importing a type, but you weren't using the preferred import type syntax for it, so it shouldn't have warned but did.

I think the issue is that you're not using the TS resolver (but neither is https://github.com/import-js/eslint-plugin-import/blob/main/config/typescript.js#L18-L22).

If you add the TS resolver - above node - does it work?

@mrdrogdrog
Copy link
Author

Oh no! I became the person in this xkcd 🙈

https://xkcd.com/1172/

@mrdrogdrog
Copy link
Author

mrdrogdrog commented Oct 17, 2021

I read the typescript and eslint docs again and changed my code to use "import type" and the "consistent-type-import" rule I mentioned above.

Maybe you should improve the error message somehow? "unable to resolve path to module" is a bit confusing if you're sitting there and think "but the file exists o.ö"

@Ulrikop
Copy link

Ulrikop commented Oct 18, 2021

I get also problems with .d.ts for import/extensions now. I believe it belongs to the problems described here, so I don' create a new bug for it.

I'm using the config 'import/extensions': ['error', 'always', { pattern: { js: 'never', ts: 'never' }, ignorePackages: true }].

If I have a file foo.d.ts without a corresponding .js file, I'm getting now errors for import like import type { ... } from './foo';.

@mrdrogdrog it's not normal to have a .d.ts file and no corresponding .js file to be imported.

That is wrong in my opinion. e.g. when I want to describe the expected messages of a communication I add them to an own file.
Or if I describe an option param of a method which is used in different files (because it is passed through or so) I want to have it also described in an own options.d.s file.

So it could be, that the most are not doing this but I want to be able to do it like I think it is good. Can we provide an option to that rules to get the old behavior again?

But for now I have to disable all the rules :-(

edit:

I see what you mean tho - in this case, you're importing a type, but you weren't using the preferred import type syntax for it, so it shouldn't have warned but did.

the import type is not possible always: when importing a const enum to use a value of it, I can not add the type

@krailler
Copy link

krailler commented Oct 18, 2021

I've the same issue as @Ulrikop . I downgraded to eslint-plugin-import@2.24.2

@ljharb
Copy link
Member

ljharb commented Oct 18, 2021

OK, to sum up:

When using the typescript resolver, and importing a type, or an enum, from a .d.ts file - whether there's a corresponding .js file or not, and whether using import type or not - it should resolve and the linter should not warn.

When not using the typescript resolver, and using typescript, your config is unsupported.

cc @jablko, it'd be great to roll forward with a fix so we don't have to revert #2220.

@jablko
Copy link
Contributor

jablko commented Oct 23, 2021

cc @jablko, it'd be great to roll forward with a fix so we don't have to revert #2220.

Thanks! I've opened #2270 aimed at the issue reported by @Ulrikop, with import/extensions rule and type-only imports.

@ljharb
Copy link
Member

ljharb commented Oct 25, 2021

@mrdrogdrog does #2270 seem like it solves your issue?

@Ulrikop
Copy link

Ulrikop commented Oct 27, 2021

the changes of @jablko does not fix all problems.

Given:

// foo.d.ts
export const Foo {
  Bar
}

Then I get the error Missing file extension for "./foo" (import/extensions) for this file:

import { Foo } from './foo';

const baz = Foo.bar;

a import type { Foo } from './foo'; does not work in that case, because TypeScript throws the error: 'Foo' cannot be used as a value because it was imported using 'import type'.ts(1361)

@ljharb
Copy link
Member

ljharb commented Oct 27, 2021

@Ulrikop are you checking with the latest master? I haven't released those fixes yet.

@Ulrikop
Copy link

Ulrikop commented Oct 27, 2021

Yes, I copied the changes of the PR #2270 into my node_modules directory.

The following previously mentioned issue is resolved with the change:

If I have a file foo.d.ts without a corresponding .js file, I'm getting now errors for import like import type { ... } from './foo';.

But since the fix on the PR only changes that an import type is not checked with the Missing file extension error, the problem remains when importing without the type keyword (like my example with a const enum).

@ljharb
Copy link
Member

ljharb commented Oct 27, 2021

thanks for confirming. cc @jablko :-)

@jablko
Copy link
Contributor

jablko commented Oct 27, 2021

Great point, thanks for the feedback!

import { Foo } from './foo';

By importing from foo.d.ts without a foo.js, you're relying on TypeScript to elide the const-enum import, if isolatedModules is false. Won't work if isolatedModules is true, but the compiler will warn about that.

There's no way for eslint-plugin-import to know whether it will be elided or not, however. import type is the usual way to guarantee elision, but as you rightly point out, it blocks value usage, including of compile-time-only const enums.

There should be a way to mark an import compile-time only, and import type should be it, I think: microsoft/TypeScript#46556

Until there's such a way, I think the best would be to mark your import compile-time only in the eyes of eslint-plugin-import at least: import { Foo } from './foo'; // eslint-disable-line import/extensions -- const enum

Alternatively you can restore the old behavior yourself by customizing the import/extensions setting, but unlike eslint-disable-line, that setting makes the plugin act like you can import .d.ts files at runtime, or like TypeScript emits .js for .d.ts files (both false) so won't warn if you accidentally export const Foo: { Bar } vs. export const enum Foo { Bar } (declare a value vs. const enum) in foo.d.ts. TypeScript won't elide that import and you'll get Error: Cannot find module './foo' at runtime.

... eslint-disable-line won't warn either, to be fair, but it's safer to add exceptions where you expect to find const enums than to your whole project, I think? Until the compiler is able to check your expectations.

@mrdrogdrog
Copy link
Author

mrdrogdrog commented Oct 28, 2021

For me this topic is done. I changed our code to use import type which is more correct. 🤷

I don't see the behavior of eslint as a bug anymore.

Thank you for your help ♥️

@jablko
Copy link
Contributor

jablko commented Oct 29, 2021

@mrdrogdrog 👍

@Ulrikop Thinking more about this, and TypeScript maintainers' amenity to value usage of type-only const enums: microsoft/TypeScript#42991 (comment)

If isolatedModules matters to you, could you rename foo.d.ts -> foo.ts? That would simultaneously pacify eslint-plugin-import. The compiler will elide import { Foo } from './foo' if isolatedModules is false, and preserve it if true.

@Oliver84

This comment has been minimized.

@ljharb

This comment has been minimized.

@Ulrikop
Copy link

Ulrikop commented Nov 3, 2021

Sorry for my late answer. due to an emergency in my family, I don't have time to test anything at the moment (e.g. adding .d.ts to config).
Even though I think that having your own rule to check whether .d.ts files are only imported as type is better, so as not to force anyone who wants to use the other rules to do so,I will find a way so that my company can use the eslint rules (patch the old behavior, find a way to configure it to work for us, change the code, ... )

@JounQin
Copy link
Collaborator

JounQin commented Nov 19, 2021

Have you tried eslint-import-resolver-typescript?

@ZJS248
Copy link

ZJS248 commented Dec 1, 2021

It did work when I use import type on my workspace , and it still post some false positives when I use
.d.ts files in node_modules ;
as example :

import type { Foo } from "./app"; // it works without errors
import type { FormItemRule } from "element-plus/es/components/form/src/form.type"; // it causes errors

this is my code

https://github.com/ZJS248/test/blob/master/src/main.ts

@jablko
Copy link
Contributor

jablko commented Dec 1, 2021

@ZJS248 Thanks for the code, I haven't tried linting it myself yet ... Can you elaborate on the false positives/it causes errors that you're seeing?

My belief is that the import/no-unresolved rule currently skips import type declarations, so either you're seeing other errors, or my belief is wrong?

@ZJS248
Copy link

ZJS248 commented Dec 2, 2021

@jablk
errors like this
include import/named import/no-extraneous-dependencies import/no-cycle etc.
screenshot
o

baumandm added a commit to ExpediaGroup/insights-explorer that referenced this issue Jan 6, 2022
dependabot bot pushed a commit to ExpediaGroup/insights-explorer that referenced this issue Jan 7, 2022
lipemat added a commit to lipemat/eslint-config that referenced this issue Feb 2, 2022
In future ESLint 8, this plugin must be enabled.

We are currently blocked from updating due to a bug in
eslint-plugin-import

@link import-js/eslint-plugin-import#2267
lipemat added a commit to lipemat/eslint-config that referenced this issue Feb 2, 2022
In future ESLint 8, this plugin must be enabled.

We are currently blocked from updating due to a bug in
eslint-plugin-import

@link import-js/eslint-plugin-import#2267
turadg added a commit to Agoric/agoric-sdk that referenced this issue Apr 22, 2022
turadg added a commit to Agoric/agoric-sdk that referenced this issue Apr 22, 2022
turadg added a commit to Agoric/agoric-sdk that referenced this issue Apr 22, 2022
turadg added a commit to Agoric/agoric-sdk that referenced this issue Apr 25, 2022
turadg added a commit to Agoric/agoric-sdk that referenced this issue Apr 26, 2022
@JounQin
Copy link
Collaborator

JounQin commented Jul 19, 2022

You should use https://github.com/import-js/eslint-import-resolver-typescript

@JounQin JounQin closed this as not planned Won't fix, can't repro, duplicate, stale Jul 19, 2022
turadg added a commit to Agoric/agoric-sdk that referenced this issue Jul 20, 2022
turadg added a commit to Agoric/agoric-sdk that referenced this issue Jul 25, 2022
turadg added a commit to Agoric/agoric-sdk that referenced this issue Jul 25, 2022
turadg added a commit to Agoric/agoric-sdk that referenced this issue Aug 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

9 participants