From a1b0a1580cafd6d264f7068014349c02f368bfb9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Eduardo=20Bou=C3=A7as?= <mail@eduardoboucas.com>
Date: Wed, 17 Nov 2021 11:52:13 +0000
Subject: [PATCH] fix: only transpile ESM imports when entrypoint is ESM (#807)

* fix: only transpile ESM imports when entrypoint is ESM

* chore: fix tests

* chore: fix test on Windows
---
 src/runtimes/node/bundlers/nft/es_modules.ts  | 25 ++++++++++++-------
 .../node-cjs-importing-mjs/function.js        |  4 +++
 .../node-cjs-importing-mjs/lib/file.mjs       | 10 ++++++++
 tests/main.js                                 | 23 +++++++++++++++++
 4 files changed, 53 insertions(+), 9 deletions(-)
 create mode 100644 tests/fixtures/node-cjs-importing-mjs/function.js
 create mode 100644 tests/fixtures/node-cjs-importing-mjs/lib/file.mjs

diff --git a/src/runtimes/node/bundlers/nft/es_modules.ts b/src/runtimes/node/bundlers/nft/es_modules.ts
index 621e26879..304c444c5 100644
--- a/src/runtimes/node/bundlers/nft/es_modules.ts
+++ b/src/runtimes/node/bundlers/nft/es_modules.ts
@@ -30,7 +30,12 @@ const patchESMPackage = async (path: string, fsCache: FsCache) => {
   return JSON.stringify(patchedPackageJson)
 }
 
-const shouldTranspile = (path: string, cache: Map<string, boolean>, reasons: NodeFileTraceReasons): boolean => {
+const shouldTranspile = (
+  path: string,
+  cache: Map<string, boolean>,
+  esmPaths: Set<string>,
+  reasons: NodeFileTraceReasons,
+): boolean => {
   if (cache.has(path)) {
     return cache.get(path) as boolean
   }
@@ -47,16 +52,18 @@ const shouldTranspile = (path: string, cache: Map<string, boolean>, reasons: Nod
 
   const { parents } = reason
 
-  // If the path doesn't have any parents, it's safe to transpile.
+  // If the path is an entrypoint, we transpile it only if it's an ESM file.
   if (parents.size === 0) {
-    cache.set(path, true)
+    const isESM = esmPaths.has(path)
 
-    return true
+    cache.set(path, isESM)
+
+    return isESM
   }
 
   // The path should be transpiled if every parent will also be transpiled, or
   // if there is no parent.
-  const shouldTranspilePath = [...parents].every((parentPath) => shouldTranspile(parentPath, cache, reasons))
+  const shouldTranspilePath = [...parents].every((parentPath) => shouldTranspile(parentPath, cache, esmPaths, reasons))
 
   cache.set(path, shouldTranspilePath)
 
@@ -77,15 +84,15 @@ const transpileESM = async ({
   reasons: NodeFileTraceReasons
 }) => {
   const cache: Map<string, boolean> = new Map()
-  const paths = [...esmPaths].filter((path) => shouldTranspile(path, cache, reasons))
-  const pathsSet = new Set(paths)
+  const pathsToTranspile = [...esmPaths].filter((path) => shouldTranspile(path, cache, esmPaths, reasons))
+  const pathsToTranspileSet = new Set(pathsToTranspile)
   const packageJsonPaths: string[] = [...reasons.entries()]
     .filter(([path, reason]) => {
       if (basename(path) !== 'package.json') {
         return false
       }
 
-      const needsPatch = [...reason.parents].some((parentPath) => pathsSet.has(parentPath))
+      const needsPatch = [...reason.parents].some((parentPath) => pathsToTranspileSet.has(parentPath))
 
       return needsPatch
     })
@@ -93,7 +100,7 @@ const transpileESM = async ({
   const rewrites = await getPatchedESMPackages(packageJsonPaths, fsCache)
 
   await Promise.all(
-    paths.map(async (path) => {
+    pathsToTranspile.map(async (path) => {
       const absolutePath = basePath ? resolve(basePath, path) : resolve(path)
       const transpiled = await transpile(absolutePath, config)
 
diff --git a/tests/fixtures/node-cjs-importing-mjs/function.js b/tests/fixtures/node-cjs-importing-mjs/function.js
new file mode 100644
index 000000000..056e814e8
--- /dev/null
+++ b/tests/fixtures/node-cjs-importing-mjs/function.js
@@ -0,0 +1,4 @@
+exports.handler = async (event, context) => {
+  const { App } = await import('./lib/file.mjs')
+  return new App(event, context)
+}
diff --git a/tests/fixtures/node-cjs-importing-mjs/lib/file.mjs b/tests/fixtures/node-cjs-importing-mjs/lib/file.mjs
new file mode 100644
index 000000000..5f1c6de1a
--- /dev/null
+++ b/tests/fixtures/node-cjs-importing-mjs/lib/file.mjs
@@ -0,0 +1,10 @@
+class App {
+  constructor(event, context) {
+    return {
+      statusCode: 200,
+      body: 'Hello world',
+    }
+  }
+}
+
+export { App }
diff --git a/tests/main.js b/tests/main.js
index 1508cc8f2..90dc90b63 100644
--- a/tests/main.js
+++ b/tests/main.js
@@ -470,6 +470,29 @@ testMany(
   },
 )
 
+testMany(
+  'Can bundle CJS functions that import ESM files with an `import()` expression',
+  ['bundler_esbuild', 'bundler_nft', 'bundler_nft_transpile'],
+  async (options, t) => {
+    const fixtureName = 'node-cjs-importing-mjs'
+    const { files, tmpDir } = await zipFixture(t, fixtureName, {
+      opts: options,
+    })
+
+    await unzipFiles(files)
+
+    const func = require(join(tmpDir, 'function.js'))
+
+    // Dynamic imports were added in Node v13.2.0.
+    if (semver.gte(nodeVersion, '13.2.0')) {
+      const { body, statusCode } = await func.handler()
+
+      t.is(body, 'Hello world')
+      t.is(statusCode, 200)
+    }
+  },
+)
+
 testMany(
   'Can require local files deeply',
   ['bundler_default', 'bundler_esbuild', 'bundler_esbuild_zisi', 'bundler_default_nft', 'bundler_nft'],