fix: handle named imports from CJS modules that use dynamic import

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.

Author: ludofischer

CHANGELOG.md

@@ -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])

src/ExportMap.js

@@ -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
@@ -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 = {};
@@ -710,6 +718,7 @@ ExportMap.parse = function (path, content, context) {
     m.namespace.set('default', {}); // add default export
   }
 
+  m.maybeNotEsm = maybeNotEsm;
   return m;
 };
 

src/rules/named.js

@@ -40,7 +40,7 @@ module.exports = {
       }
 
       const imports = Exports.get(node.source.value, context);
-      if (imports == null) {
+      if (imports == null || imports.maybeNotEsm) {
         return;
       }
 
@@ -97,6 +97,7 @@ module.exports = {
         // return if it's not a string source
         || source.type !== 'Literal'
         || variableExports == null
+        || variableExports.maybeNotEsm
       ) {
         return;
       }

tests/files/dynamic-import-in-commonjs.js

@@ -0,0 +1,5 @@
+async function doSomething() {
+  await import('./bar.js');
+}
+
+exports.something = 'hello';

tests/src/rules/named.js

@@ -32,6 +32,9 @@ ruleTester.run('named', rule, {
     test({ code: 'import { jsxFoo } from "./jsx/AnotherComponent"',
       settings: { 'import/resolve': { 'extensions': ['.js', '.jsx'] } } }),
 
+    test({ code: 'import { something } from "./dynamic-import-in-commonjs"',
+      parserOptions: { ecmaVersion: 2021 } }),
+
     // validate that eslint-disable-line silences this properly
     test({ code: 'import {a, b, d} from "./common"; ' +
                 '// eslint-disable-line named' }),
@@ -165,6 +168,11 @@ ruleTester.run('named', rule, {
       options: [{ commonjs: true }],
     }),
 
+    test({ code: 'const { something } = require("./dynamic-import-in-commonjs")',
+      parserOptions: { ecmaVersion: 2021 },
+      options: [{ commonjs: true }],
+    }),
+    
     test({
       code: 'const { baz } = require("./bar")',
       errors: [error('baz', './bar')],