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] named/ExportMap: handle named imports from CJS modules that use dynamic import #2341

Merged
merged 1 commit into from
Jan 1, 2022

Conversation

ludofischer
Copy link
Contributor

Fix #2294

  • Modify ExportMap to track whether the module is unambiguously ES6
  • Add tests for named import with import as require()

Proposed solution

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.

Previous to 7c382f0 this was not necessary because a null ExportMap was returned for all ambiguous modules.

Alternatives

Parse the CommonJS exports instead of ignoring the CommonJS modules. Rejected because analysingg CommonJS exports seems out of scope of this plugin.

… use dynamic import

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.
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests look good!

src/ExportMap.js Outdated Show resolved Hide resolved
src/ExportMap.js Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Dec 30, 2021

Codecov Report

Merging #2341 (56b6838) into main (ef980d4) will increase coverage by 0.07%.
The diff coverage is 100.00%.

❗ Current head 56b6838 differs from pull request most recent head e3ca68e. Consider uploading reports for the commit e3ca68e to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2341      +/-   ##
==========================================
+ Coverage   94.64%   94.72%   +0.07%     
==========================================
  Files          65       65              
  Lines        2691     2690       -1     
  Branches      891      893       +2     
==========================================
+ Hits         2547     2548       +1     
+ Misses        144      142       -2     
Impacted Files Coverage Δ
src/ExportMap.js 100.00% <ø> (+7.14%) ⬆️
src/rules/named.js 84.90% <100.00%> (+2.21%) ⬆️

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 ef980d4...e3ca68e. Read the comment docs.

@ludofischer
Copy link
Contributor Author

I've applied your suggestion of turning maybeNotEsm into a parseGoal field. Is there another refactor you had in mind where this parseGoal should be used? Another thing that was on my mind is that the default rule might also fail in a similar case.

@ljharb
Copy link
Member

ljharb commented Dec 30, 2021

I'm mainly hoping that we can get these kinds of helpers in a place where it becomes trivial to support CJS files as well as ESM files.

What are your thoughts about having an "empty" ExportsMap instead of null for a CJS file without dynamic imports?

@ludofischer
Copy link
Contributor Author

What are your thoughts about having an "empty" ExportsMap instead of null for a CJS file without dynamic imports?

I need to take a longer look at the code. My instinct would be that it increases the risk of breaking something else as we'll miss one spot that's relying on the ExportsMap being null.

@ljharb
Copy link
Member

ljharb commented Dec 31, 2021

Fair enough, let's do that in a follow-up PR then.

@ljharb ljharb changed the title fix: handle named imports from CJS modules that use dynamic import [Fix] named/ExportMap: handle named imports from CJS modules that use dynamic import Dec 31, 2021
@ludofischer
Copy link
Contributor Author

Fair enough, let's do that in a follow-up PR then.

I feel like I am missing some context to make a follow-up. I've looked at the implementation for the rules some more and I've also looked at the issues. I cannot find case where returning empty ExportMap from a CJS file would make a difference:

  1. it would not get rid of returned null since ExportMap.get() returns null when the file is ignored or has an invalid extension according to the config
  2. all of dependencies, imports, errors would be empty, it would be the same as skipping processing completely, which already happens when ExportMap is null

On an unrelated note, have you thought about using issue templates to require a link to a reproduction? I've got the impression that a lot of open issues are unactionable because they lack a reproduction.

@ljharb ljharb merged commit e3ca68e into import-js:main Jan 1, 2022
@ljharb
Copy link
Member

ljharb commented Jan 1, 2022

@ludofischer i'm mainly thinking that it'd make it clearer for rules to use ExportsMap - since CJS Scripts have a dep graph too, and this plugin should support them.

As for an issue template, I'd be happy to look at a PR that added one :-)

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

Successfully merging this pull request may close these issues.

import/named falsely reports import as not found (regression in 2.25)
2 participants