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/named falsely reports import as not found (regression in 2.25) #2294

Closed
ludofischer opened this issue Nov 5, 2021 · 16 comments · Fixed by #2341
Closed

import/named falsely reports import as not found (regression in 2.25) #2294

ludofischer opened this issue Nov 5, 2021 · 16 comments · Fixed by #2341

Comments

@ludofischer
Copy link
Contributor

eslint-plugin-import version: 2.25.0 and 2.25.2
eslint version: 7.32 and 8.1
Reproduction repo: https://github.com/ludofischer/eslint-plugin-import-error-demo

When using the optimize named import from the svgo 2.8 library, since 2.25 eslint-plugin-import reports

error  optimize not found in 'svgo'  import/named

while the import was correctly handled as valid in eslint-plugin-import 2.24. svgo uses CommonJS with just a main package.json field that points to this file: https://github.com/svg/svgo/blob/master/lib/svgo-node.js

@ludofischer
Copy link
Contributor Author

With 2.25.2, even the default import triggers a false positive:

import svgo from 'svgo';

yields

error  No default export found in imported module "svgo"  import/default

But it works with eslint-plugin-import 2.24

@ludofischer
Copy link
Contributor Author

ludofischer commented Nov 5, 2021

I've also found a simpler way to reproduce the error: (new commit in same repo https://github.com/ludofischer/eslint-plugin-import-error-demo).
Use an ES import from a CommonJS module when the CommonJS module contains an ES-style dynamic import. In that case, eslint-plugin-import will:

  1. try to analyze the CommonJS module
  2. fail to understand to recognize either a default import or named import from the CommonJS module

Removing the dynamic import from the code of the CommonJS module removes the eslint warning.
If I understand the docs correctly, eslint-plugin-import should not even attempt to analyze imports from svgo, since the svgo package does not contain a module field. Also a CommonJS module with a dynamic import in it is likely not 'unambiguously an ES', so I think eslint import plugin should not attempt to analyze it.

@ljharb
Copy link
Member

ljharb commented Nov 6, 2021

A module field shouldn't be required - it's not even a standard field. It's just something we support for convenience.

https://unpkg.com/browse/svgo@2.8.0/package.json does indeed point to https://unpkg.com/browse/svgo@2.8.0/lib/svgo-node.js.

Thanks for the repro repo; I'll check it out.

@ludofischer
Copy link
Contributor Author

Wouldn't the expected behaviour to avoid analysing SVGO by default since it's not ESM? In the second commit of repro repo you should see that removing the dyanmic import() in the imported test.js makes the linting pass.

@ljharb
Copy link
Member

ljharb commented Nov 6, 2021

There's a commonjs: true rule option, so we need the capability anyways, but also CJS is just as statically analyzeable as ESM, so we should be able to handle it.

I see that removing the import() makes the linting pass, but its presence shouldn't cause a passing test to fail - it should either have no impact, or should cause a failing test to pass, by "using" more imports.

@ludofischer
Copy link
Contributor Author

The named rule docs say

A module path that is ignored or not unambiguously an ES module will not be reported when imported

That made me think the issue in 2.25 is not ignoring the import from a CJS module.

@shamilovtim

This comment has been minimized.

@ljharb

This comment has been minimized.

@shamilovtim

This comment has been minimized.

@BalzGuenat
Copy link

This is still an issue for us after we tried to update the dependencies of our React app. The workaround of using a type import worked when linting via yarn eslint but did not work when compiling/linting via react-scripts start. We had to downgrade to 2.24.2 for it to work.

@ljharb
Copy link
Member

ljharb commented Nov 30, 2021

@BalzGuenat please file a bug on CRA if it doesn't support import type.

@BalzGuenat
Copy link

To be clear: react-scripts start fails because of this regression. We could work around it for yarn eslint but the same workaround does not work for the linting that happens as part of react-scripts start.

@ljharb
Copy link
Member

ljharb commented Dec 15, 2021

I continue to be happy to review a PR that fixes the regression - that "use import type" is the best practice and will help you in the long run doesn't change that it's still a bug.

@ludofischer
Copy link
Contributor Author

In the case I posted the reason it works in 2.24 is because 2.24 constructs a null ExportMap. So in a sense 2.25 is an improvement in that at least it manages to construct the ExportMap, unfortunately it seems the wrong one.

@ludofischer
Copy link
Contributor Author

I think I have found the source of the bug but I have no idea how to fix it.

I believe the problem occurred in commit 7c382f0, line 408. In order to analyze dynamic imports, this commit does not return null any more if a module contains dynamic imports, even if the module fails the unambiguous.isModule() check. It assumes that any module that contains dynamic imports is in fact an ES module.

Now I think in the case I've submitted the expected behaviour would be to return null. The reason is that I can't see any logic to parse CommonJS exports, so I think the intention is to always skip checking if the exporting module is CommonJS.

The error occurs when trying to import from a CommonJS module that uses dynamic import(). I assume the analysis would need to bail out in case there are dynamic imports together with CommonJS exports, but I cannot find logic to detect CommonJS exports. Has anyone got any other ideas?

ludofischer added a commit to ludofischer/eslint-plugin-import that referenced this issue Dec 30, 2021
Fix import-js#2294

Mark ambiguous (i.e. not unambiguously ESM) modules that contain dynamic import()
so that they can be ignored like other ambiguous modules when analysing named imports.

In this way, the named rule accepts named imports from CJS modules that contain
dynamic imports.

Also fix the case where the importing module uses a require() call.
ludofischer added a commit to ludofischer/eslint-plugin-import that referenced this issue Dec 30, 2021
Fix import-js#2294

Mark ambiguous (i.e. not unambiguously ESM) modules that contain dynamic import()
so that they can be ignored like other ambiguous modules when analysing named imports.

In this way, the named rule accepts named imports from CJS modules that contain
dynamic imports.

Also fix the case where the importing module uses a require() call.
ludofischer added a commit to ludofischer/eslint-plugin-import that referenced this issue Dec 30, 2021
Fix import-js#2294

Mark ambiguous (i.e. not unambiguously ESM) modules that contain dynamic import()
so that they can be ignored like other ambiguous modules when analysing named imports.

In this way, the named rule accepts named imports from CJS modules that contain
dynamic imports.

Also fix the case where the importing module uses a require() call.
ludofischer added a commit to ludofischer/eslint-plugin-import that referenced this issue Dec 30, 2021
Fix import-js#2294

Mark ambiguous (i.e. not unambiguously ESM) modules that contain dynamic import()
so that they can be ignored like other ambiguous modules when analysing named imports.

In this way, the named rule accepts named imports from CJS modules that contain
dynamic imports.

Also fix the case where the importing module uses a require() call.
@ludofischer
Copy link
Contributor Author

I've opened #2341 that should fix at least the case I've reported.

@ljharb ljharb closed this as completed in e3ca68e Jan 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants