Skip to content

Commit

Permalink
fix: handle named imports from CJS modules that use dynamic import
Browse files Browse the repository at this point in the history
Fix #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.
  • Loading branch information
ludofischer committed Dec 30, 2021
1 parent ef980d4 commit 0dba1cf
Show file tree
Hide file tree
Showing 5 changed files with 28 additions and 3 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ This change log adheres to standards from [Keep a CHANGELOG](https://keepachange
- `importType`: avoid crashing on a non-string' ([#2305], thanks [@ljharb])
- [`first`]: prevent crash when parsing angular templates ([#2210], thanks [@ljharb])
- `importType`: properly resolve `@/*`-aliased imports as internal ([#2334], thanks [@ombene])
- `named`: avoid false positives when importing from a CommonJS module that uses `import()` ([#2341], thanks [@ludofischer])

### Changed
- [`no-default-import`]: report on the token "default" instead of the entire node ([#2299], thanks [@pmcelhaney])
Expand Down
11 changes: 10 additions & 1 deletion src/ExportMap.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,13 @@ export default class ExportMap {
*/
this.imports = new Map();
this.errors = [];
/**
* true when we are still unsure if
* the module is ESM, happens when the module
* contains dynamic `import()` but no other
* ESM import/export
*/
this.maybeNotEsm = false;
}

get hasDefault() { return this.get('default') != null; } // stronger than this.has
Expand Down Expand Up @@ -406,7 +413,8 @@ ExportMap.parse = function (path, content, context) {
},
});

if (!unambiguous.isModule(ast) && !hasDynamicImports) return null;
const maybeNotEsm = !unambiguous.isModule(ast);
if (maybeNotEsm && !hasDynamicImports) return null;

const docstyle = (context.settings && context.settings['import/docstyle']) || ['jsdoc'];
const docStyleParsers = {};
Expand Down Expand Up @@ -710,6 +718,7 @@ ExportMap.parse = function (path, content, context) {
m.namespace.set('default', {}); // add default export
}

m.maybeNotEsm = maybeNotEsm;
return m;
};

Expand Down
3 changes: 2 additions & 1 deletion src/rules/named.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ module.exports = {
}

const imports = Exports.get(node.source.value, context);
if (imports == null) {
if (imports == null || imports.maybeNotEsm) {
return;
}

Expand Down Expand Up @@ -97,6 +97,7 @@ module.exports = {
// return if it's not a string source
|| source.type !== 'Literal'
|| variableExports == null
|| variableExports.maybeNotEsm
) {
return;
}
Expand Down
5 changes: 5 additions & 0 deletions tests/files/dynamic-import-in-commonjs.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
async function doSomething() {
await import('./bar.js');
}

exports.something = 'hello';
11 changes: 10 additions & 1 deletion tests/src/rules/named.js
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,16 @@ ruleTester.run('named', rule, {
sourceType: 'module',
ecmaVersion: 2020,
},
})) || []),
})),

testVersion('>=7.8.0', () =>({ code: 'const { something } = require("./dynamic-import-in-commonjs")',
parserOptions: { ecmaVersion: 2021 },
options: [{ commonjs: true }],
})),

testVersion('>=7.8.0', () => ({ code: 'import { something } from "./dynamic-import-in-commonjs"',
parserOptions: { ecmaVersion: 2021 } })),
),
],

invalid: [
Expand Down

0 comments on commit 0dba1cf

Please sign in to comment.