Repository: alangpierce/sucrase (ts)

Issue:
Dynamic import not act as expected as typescript transpiled
Here is the example:

https://sucrase.io/#compareWithBabel=false&compareWithTypeScript=true&code=import%20b%20from%20'a'%3B%0A%0Aasync%20function%20a%20()%20%7B%0A%20%20console.log(b)%3B%0A%20%20const%20c%20%3D%20await%20import('a')%3B%0A%20%20console.log(c)%3B%0A%20%20console.log(c.default)%3B%0A%7D

While static import is transpiled to use `_interopRequireDefault`, but dynamic import is not transpiled to use `__importStar`, so in runtime `c.default` is not defined.

Pull request description:
Fix dynamic import on plain CJS files
Fixes #778

Previously, `import()` transpiled to a promise-wrapped `require`, which worked fine for ESM-to-CJS-transpiled modules, but when importing plain CJS modules, the behavior was inconsistent with other implementations. In that case, we needed to call `_interopRequireWildcard` to nest the module under a `default` key.

I tested to confirm that this updated behavior is consistent with Node ESM-to-CJS dynamic import, as well as Babel, TypeScript, and swc, so this change will be considered a bug fix rather than a breaking change, even though it is possible that existing use cases may have been relying on the old behavior.

Patch:
diff --git a/src/transformers/CJSImportTransformer.ts b/src/transformers/CJSImportTransformer.ts
index 3750ab8..afbbdc9 100644
--- a/src/transformers/CJSImportTransformer.ts
+++ b/src/transformers/CJSImportTransformer.ts
@@ -1,4 +1,5 @@
 import type CJSImportProcessor from "../CJSImportProcessor";
+import type {HelperManager} from "../HelperManager";
 import type NameManager from "../NameManager";
 import {IdentifierRole, isDeclaration, isObjectShorthandDeclaration} from "../parser/tokenizer";
 import {ContextualKeyword} from "../parser/tokenizer/keywords";
@@ -30,6 +31,7 @@ export default class CJSImportTransformer extends Transformer {
     readonly tokens: TokenProcessor,
     readonly importProcessor: CJSImportProcessor,
     readonly nameManager: NameManager,
+    readonly helperManager: HelperManager,
     readonly reactHotLoaderTransformer: ReactHotLoaderTransformer | null,
     readonly enableLegacyBabel5ModuleInterop: boolean,
     readonly isTypeScriptTransformEnabled: boolean,
@@ -122,7 +124,10 @@ export default class CJSImportTransformer extends Transformer {
         this.tokens.copyToken();
         return;
       }
-      this.tokens.replaceToken("Promise.resolve().then(() => require");
+      const interopRequireWildcardName = this.helperManager.getHelperName("interopRequireWildcard");
+      this.tokens.replaceToken(
+        `Promise.resolve().then(() => ${interopRequireWildcardName}(require`,
+      );
       const contextId = this.tokens.currentToken().contextId;
       if (contextId == null) {
         throw new Error("Expected context ID on dynamic import invocation.");
@@ -131,7 +136,7 @@ export default class CJSImportTransformer extends Transformer {
       while (!this.tokens.matchesContextIdAndLabel(tt.parenR, contextId)) {
         this.rootTransformer.processToken();
       }
-      this.tokens.replaceToken("))");
+      this.tokens.replaceToken(")))");
       return;
     }
 
diff --git a/src/transformers/RootTransformer.ts b/src/transformers/RootTransformer.ts
index 3326080..850cd36 100644
--- a/src/transformers/RootTransformer.ts
+++ b/src/transformers/RootTransformer.ts
@@ -83,6 +83,7 @@ export default class RootTransformer {
           tokenProcessor,
           importProcessor,
           this.nameManager,
+          this.helperManager,
           reactHotLoaderTransformer,
           enableLegacyBabel5ModuleInterop,
           transforms.includes("typescript"),


Test patch:
diff --git a/integration-test/integration-tests.ts b/integration-test/integration-tests.ts
index 1892c34..2e0bf01 100644
--- a/integration-test/integration-tests.ts
+++ b/integration-test/integration-tests.ts
@@ -30,6 +30,19 @@ describe("integration tests", () => {
     }
   }
 
+  /**
+   * Find sucrase/register integration tests.
+   *
+   * Each test has a file starting with "main" (e.g. main.ts, main.tsx,
+   * main.mts, etc) that's used as the entry point. The test should be written
+   * in such a way that the execution throws an exception if the test fails.
+   */
+  for (const testFile of discoverTests("test-cases/register-cases", "main")) {
+    it(testFile, async () => {
+      await execPromise(`node -r ${__dirname}/../register ${testFile}`);
+    });
+  }
+
   /**
    * Find Jest integration tests.
    *
diff --git a/integration-test/test-cases/register-cases/allows-dynamic-import/main.js b/integration-test/test-cases/register-cases/allows-dynamic-import/main.js
new file mode 100644
index 0000000..b12ab85
--- /dev/null
+++ b/integration-test/test-cases/register-cases/allows-dynamic-import/main.js
@@ -0,0 +1,14 @@
+async function main() {
+  const plainCJSFile = await import("./plain-cjs-file");
+  const transpiledESMFile = await import("./transpiled-esm-file");
+  if (plainCJSFile.default !== 15) {
+    throw new Error();
+  }
+  if (transpiledESMFile.a !== 1) {
+    throw new Error();
+  }
+  if (transpiledESMFile.default !== 3) {
+    throw new Error();
+  }
+}
+main();
diff --git a/integration-test/test-cases/register-cases/allows-dynamic-import/plain-cjs-file.js b/integration-test/test-cases/register-cases/allows-dynamic-import/plain-cjs-file.js
new file mode 100644
index 0000000..d78ec6d
--- /dev/null
+++ b/integration-test/test-cases/register-cases/allows-dynamic-import/plain-cjs-file.js
@@ -0,0 +1,1 @@
+module.exports = 15;
diff --git a/integration-test/test-cases/register-cases/allows-dynamic-import/transpiled-esm-file.js b/integration-test/test-cases/register-cases/allows-dynamic-import/transpiled-esm-file.js
new file mode 100644
index 0000000..201c670
--- /dev/null
+++ b/integration-test/test-cases/register-cases/allows-dynamic-import/transpiled-esm-file.js
@@ -0,0 +1,2 @@
+export const a = 1;
+export default 3;
diff --git a/test/imports-test.ts b/test/imports-test.ts
index 89030b1..e1d76a8 100644
--- a/test/imports-test.ts
+++ b/test/imports-test.ts
@@ -1055,9 +1055,9 @@ module.exports = exports.default;
         const foo = await import('foo');
       }
     `,
-      `"use strict";
+      `"use strict";${IMPORT_WILDCARD_PREFIX}
       async function loadThing() {
-        const foo = await Promise.resolve().then(() => require('foo'));
+        const foo = await Promise.resolve().then(() => _interopRequireWildcard(require('foo')));
       }
     `,
     );