Repository: netlify/zip-it-and-ship-it
Pull request #458 (2021-05-07 15:42:21)

feat: add `nodeModulesWithDynamicImports` property
**- Summary**

With this PR, zip-it-and-ship-it will cycle through the warnings produced by esbuild and look for any instances of dynamic imports. It compiles a list of all the modules in this situation and returns it in a `nodeModulesWithDynamicImports` property.

We'll be able to use this information in both Netlify Build and CLI to show an informative warning message, advising people to add the modules to the `external_node_modules` array if they see any runtime issues.

Unfortunately, to make this happen we need to pin esbuild to version 0.11.10, since version 0.11.11 removed these warnings.

**- Test plan**

Added a new test.

**- A picture of a cute animal (not mandatory but encouraged)**

![EV7Kbx_WoAAgB7L](https://user-images.githubusercontent.com/4162329/117474632-079fed80-af53-11eb-952e-1909b7eaf782.jpg)


github-actions[bot]: ## ⏱ Benchmark results

Comparing with 5823fd39617e6844bfaf81bf6693400d6b0ffe82

- **largeDepsEsbuild**: 12.6s ⬆️ (5.16% increase)
- **largeDepsZisi**: 1m 16.5s ⬆️ (1.49% increase)
<!--delta:benchmark@{"largeDepsEsbuild":12612.822388666667,"largeDepsZisi":76512.28306733334}-->

Patch:
diff --git a/package-lock.json b/package-lock.json
index 2d085f8..14a3ea6 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -16,7 +16,7 @@
         "del": "^5.1.0",
         "elf-cam": "^0.1.1",
         "end-of-stream": "^1.4.4",
-        "esbuild": "^0.11.0",
+        "esbuild": "0.11.10",
         "filter-obj": "^2.0.1",
         "find-up": "^4.1.0",
         "glob": "^7.1.6",
@@ -4302,9 +4302,9 @@
       "dev": true
     },
     "node_modules/esbuild": {
-      "version": "0.11.20",
-      "resolved": "https://registry.npmjs.org/esbuild/-/esbuild-0.11.20.tgz",
-      "integrity": "sha512-QOZrVpN/Yz74xfat0H6euSgn3RnwLevY1mJTEXneukz1ln9qB+ieaerRMzSeETpz/UJWsBMzRVR/andBht5WKw==",
+      "version": "0.11.10",
+      "resolved": "https://registry.npmjs.org/esbuild/-/esbuild-0.11.10.tgz",
+      "integrity": "sha512-XvGbf+UreVFA24Tlk6sNOqNcvF2z49XAZt4E7A4H80+yqn944QOLTTxaU0lkdYNtZKFiITNea+VxmtrfjvnLPA==",
       "hasInstallScript": true,
       "bin": {
         "esbuild": "bin/esbuild"
@@ -15885,9 +15885,9 @@
       "dev": true
     },
     "esbuild": {
-      "version": "0.11.20",
-      "resolved": "https://registry.npmjs.org/esbuild/-/esbuild-0.11.20.tgz",
-      "integrity": "sha512-QOZrVpN/Yz74xfat0H6euSgn3RnwLevY1mJTEXneukz1ln9qB+ieaerRMzSeETpz/UJWsBMzRVR/andBht5WKw=="
+      "version": "0.11.10",
+      "resolved": "https://registry.npmjs.org/esbuild/-/esbuild-0.11.10.tgz",
+      "integrity": "sha512-XvGbf+UreVFA24Tlk6sNOqNcvF2z49XAZt4E7A4H80+yqn944QOLTTxaU0lkdYNtZKFiITNea+VxmtrfjvnLPA=="
     },
     "escalade": {
       "version": "3.1.1",
diff --git a/package.json b/package.json
index 9fa6f96..94403d2 100644
--- a/package.json
+++ b/package.json
@@ -61,7 +61,7 @@
     "del": "^5.1.0",
     "elf-cam": "^0.1.1",
     "end-of-stream": "^1.4.4",
-    "esbuild": "^0.11.0",
+    "esbuild": "0.11.10",
     "filter-obj": "^2.0.1",
     "find-up": "^4.1.0",
     "glob": "^7.1.6",
diff --git a/src/runtimes/node/bundler.js b/src/runtimes/node/bundler.js
index e579098..1271a72 100644
--- a/src/runtimes/node/bundler.js
+++ b/src/runtimes/node/bundler.js
@@ -7,6 +7,7 @@ const semver = require('semver')
 const { safeUnlink } = require('../../utils/fs')
 
 const { getBundlerTarget } = require('./bundler_target')
+const { getModulesWithDynamicImports } = require('./dynamic_imports')
 const { externalNativeModulesPlugin } = require('./native_modules/plugin')
 
 const supportsAsyncAPI = semver.satisfies(process.version, '>=9.x')
@@ -16,6 +17,7 @@ const supportsAsyncAPI = semver.satisfies(process.version, '>=9.x')
 // eslint-disable-next-line node/no-sync
 const buildFunction = supportsAsyncAPI ? esbuild.build : esbuild.buildSync
 
+// eslint-disable-next-line max-statements
 const bundleJsFile = async function ({
   additionalModulePaths,
   config,
@@ -24,6 +26,7 @@ const bundleJsFile = async function ({
   externalModules = [],
   ignoredModules = [],
   name,
+  srcDir,
   srcFile,
 }) {
   // De-duping external and ignored modules.
@@ -58,12 +61,14 @@ const bundleJsFile = async function ({
     const sourcemapPath = getSourcemapPath(metafile.outputs)
     const inputs = Object.keys(metafile.inputs).map((path) => resolve(path))
     const cleanTempFiles = getCleanupFunction(bundlePath, sourcemapPath)
+    const nodeModulesWithDynamicImports = await getModulesWithDynamicImports({ srcDir, warnings })
 
     return {
       bundlePath,
       cleanTempFiles,
       inputs,
       nativeNodeModules,
+      nodeModulesWithDynamicImports,
       sourcemapPath,
       warnings,
     }
diff --git a/src/runtimes/node/dynamic_imports.js b/src/runtimes/node/dynamic_imports.js
new file mode 100644
index 0000000..19c5745
--- /dev/null
+++ b/src/runtimes/node/dynamic_imports.js
@@ -0,0 +1,44 @@
+const { basename, dirname, join } = require('path')
+
+const findUp = require('find-up')
+const readPackageJson = require('read-package-json-fast')
+
+const getModulesWithDynamicImports = async ({ srcDir, warnings }) => {
+  const dynamicImportWarnings = warnings
+    // Unfortunately, esbuild doesn't give us any error codes, so this is the
+    // only way we have to single out this case.
+    .filter(({ text }) => text.includes('will not be bundled because the argument is not a string literal'))
+    .map(async ({ location }) => {
+      try {
+        const packageJsonPath = await findUp(
+          async (directory) => {
+            // We stop traversing if we're about to leave the boundaries of the
+            // function directory or any node_modules directory.
+            if (directory === srcDir || basename(directory) === 'node_modules') {
+              return findUp.stop
+            }
+
+            const path = join(directory, 'package.json')
+            const hasPackageJson = await findUp.exists(path)
+
+            return hasPackageJson ? path : false
+          },
+          { cwd: dirname(location.file) },
+        )
+
+        if (packageJsonPath !== undefined) {
+          const { name } = await readPackageJson(packageJsonPath)
+
+          return name
+        }
+      } catch (_) {
+        // We couldn't find a `package.json` or we couldn't get a package name
+        // from it. Either way, it's a no-op.
+      }
+    })
+  const packageNames = await Promise.all(dynamicImportWarnings)
+
+  return [...new Set(packageNames.filter(Boolean))]
+}
+
+module.exports = { getModulesWithDynamicImports }
diff --git a/src/runtimes/node/zip_esbuild.js b/src/runtimes/node/zip_esbuild.js
index 816eb0b..4da1c8a 100644
--- a/src/runtimes/node/zip_esbuild.js
+++ b/src/runtimes/node/zip_esbuild.js
@@ -54,6 +54,7 @@ const zipEsbuild = async ({
     cleanTempFiles,
     inputs,
     nativeNodeModules = {},
+    nodeModulesWithDynamicImports,
     sourcemapPath,
     warnings,
   } = await bundleJsFile({
@@ -64,6 +65,7 @@ const zipEsbuild = async ({
     externalModules,
     ignoredModules,
     name,
+    srcDir,
     srcFile: mainFile,
   })
   const bundlerWarnings = warnings.length === 0 ? undefined : warnings
@@ -106,7 +108,15 @@ const zipEsbuild = async ({
       srcFiles: [...supportingSrcFiles, bundlePath, ...(sourcemapPath ? [sourcemapPath] : [])],
     })
 
-    return { bundler: JS_BUNDLER_ESBUILD, bundlerWarnings, config, inputs, nativeNodeModules, path }
+    return {
+      bundler: JS_BUNDLER_ESBUILD,
+      bundlerWarnings,
+      config,
+      inputs,
+      nativeNodeModules,
+      nodeModulesWithDynamicImports,
+      path,
+    }
   } finally {
     await cleanTempFiles()
   }
diff --git a/src/zip.js b/src/zip.js
index a6d4830..f62bdc5 100644
--- a/src/zip.js
+++ b/src/zip.js
@@ -28,6 +28,7 @@ const formatZipResult = (result) => {
     mainFile,
     name,
     nativeNodeModules,
+    nodeModulesWithDynamicImports,
     path,
     runtime,
   } = result
@@ -41,6 +42,7 @@ const formatZipResult = (result) => {
     mainFile,
     name,
     nativeNodeModules,
+    nodeModulesWithDynamicImports,
     path,
     runtime: runtime.name,
   })


Test patch:
diff --git a/tests/fixtures/node-module-dynamic-import-invalid/function.js b/tests/fixtures/node-module-dynamic-import-invalid/function.js
new file mode 100644
index 0000000..41fa369
--- /dev/null
+++ b/tests/fixtures/node-module-dynamic-import-invalid/function.js
@@ -0,0 +1,1 @@
+module.exports = require('@org/test')
diff --git a/tests/fixtures/node-module-dynamic-import-invalid/node_modules/@org/test/files/one.json b/tests/fixtures/node-module-dynamic-import-invalid/node_modules/@org/test/files/one.json
new file mode 100644
index 0000000..34204b5
--- /dev/null
+++ b/tests/fixtures/node-module-dynamic-import-invalid/node_modules/@org/test/files/one.json
@@ -0,0 +1,1 @@
+{"bool": true}
diff --git a/tests/fixtures/node-module-dynamic-import-invalid/node_modules/@org/test/index.js b/tests/fixtures/node-module-dynamic-import-invalid/node_modules/@org/test/index.js
new file mode 100644
index 0000000..88247c6
--- /dev/null
+++ b/tests/fixtures/node-module-dynamic-import-invalid/node_modules/@org/test/index.js
@@ -0,0 +1,10 @@
+const number = 'one'
+const test2 = require('test-two')
+
+module.exports = () => {
+  test2()
+
+  const file = require(`./files/${number}.json`)
+
+  return file.bool
+}
diff --git a/tests/fixtures/node-module-dynamic-import-invalid/node_modules/test-two/files/one.json b/tests/fixtures/node-module-dynamic-import-invalid/node_modules/test-two/files/one.json
new file mode 100644
index 0000000..34204b5
--- /dev/null
+++ b/tests/fixtures/node-module-dynamic-import-invalid/node_modules/test-two/files/one.json
@@ -0,0 +1,1 @@
+{"bool": true}
diff --git a/tests/fixtures/node-module-dynamic-import-invalid/node_modules/test-two/index.js b/tests/fixtures/node-module-dynamic-import-invalid/node_modules/test-two/index.js
new file mode 100644
index 0000000..4e07ac5
--- /dev/null
+++ b/tests/fixtures/node-module-dynamic-import-invalid/node_modules/test-two/index.js
@@ -0,0 +1,6 @@
+module.exports = () => {
+  const number = 'one'
+  const file = require.resolve(`./files/${number}.json`)
+
+  return require(file)
+}
diff --git a/tests/fixtures/node-module-dynamic-import-invalid/package.json b/tests/fixtures/node-module-dynamic-import-invalid/package.json
new file mode 100644
index 0000000..c0b4a81
--- /dev/null
+++ b/tests/fixtures/node-module-dynamic-import-invalid/package.json
@@ -0,0 +1,7 @@
+{
+  "name": "node-module-dynamic-import",
+  "version": "1.0.0",
+  "dependencies": {
+    "@org/test": "1.0.0"
+  }
+}
diff --git a/tests/fixtures/node-module-dynamic-import/function.js b/tests/fixtures/node-module-dynamic-import/function.js
new file mode 100644
index 0000000..41fa369
--- /dev/null
+++ b/tests/fixtures/node-module-dynamic-import/function.js
@@ -0,0 +1,1 @@
+module.exports = require('@org/test')
diff --git a/tests/fixtures/node-module-dynamic-import/node_modules/@org/test/files/one.json b/tests/fixtures/node-module-dynamic-import/node_modules/@org/test/files/one.json
new file mode 100644
index 0000000..34204b5
--- /dev/null
+++ b/tests/fixtures/node-module-dynamic-import/node_modules/@org/test/files/one.json
@@ -0,0 +1,1 @@
+{"bool": true}
diff --git a/tests/fixtures/node-module-dynamic-import/node_modules/@org/test/index.js b/tests/fixtures/node-module-dynamic-import/node_modules/@org/test/index.js
new file mode 100644
index 0000000..88247c6
--- /dev/null
+++ b/tests/fixtures/node-module-dynamic-import/node_modules/@org/test/index.js
@@ -0,0 +1,10 @@
+const number = 'one'
+const test2 = require('test-two')
+
+module.exports = () => {
+  test2()
+
+  const file = require(`./files/${number}.json`)
+
+  return file.bool
+}
diff --git a/tests/fixtures/node-module-dynamic-import/node_modules/@org/test/package.json b/tests/fixtures/node-module-dynamic-import/node_modules/@org/test/package.json
new file mode 100644
index 0000000..f0130cd
--- /dev/null
+++ b/tests/fixtures/node-module-dynamic-import/node_modules/@org/test/package.json
@@ -0,0 +1,7 @@
+{
+  "name": "@org/test",
+  "version": "1.0.0",
+  "dependencies": {
+    "test-two": "1.0.0"
+  }
+}
diff --git a/tests/fixtures/node-module-dynamic-import/node_modules/test-two/files/one.json b/tests/fixtures/node-module-dynamic-import/node_modules/test-two/files/one.json
new file mode 100644
index 0000000..34204b5
--- /dev/null
+++ b/tests/fixtures/node-module-dynamic-import/node_modules/test-two/files/one.json
@@ -0,0 +1,1 @@
+{"bool": true}
diff --git a/tests/fixtures/node-module-dynamic-import/node_modules/test-two/index.js b/tests/fixtures/node-module-dynamic-import/node_modules/test-two/index.js
new file mode 100644
index 0000000..4e07ac5
--- /dev/null
+++ b/tests/fixtures/node-module-dynamic-import/node_modules/test-two/index.js
@@ -0,0 +1,6 @@
+module.exports = () => {
+  const number = 'one'
+  const file = require.resolve(`./files/${number}.json`)
+
+  return require(file)
+}
diff --git a/tests/fixtures/node-module-dynamic-import/node_modules/test-two/package.json b/tests/fixtures/node-module-dynamic-import/node_modules/test-two/package.json
new file mode 100644
index 0000000..bcba0fe
--- /dev/null
+++ b/tests/fixtures/node-module-dynamic-import/node_modules/test-two/package.json
@@ -0,0 +1,4 @@
+{
+  "name": "test-two",
+  "version": "1.0.0"
+}
diff --git a/tests/fixtures/node-module-dynamic-import/package.json b/tests/fixtures/node-module-dynamic-import/package.json
new file mode 100644
index 0000000..c0b4a81
--- /dev/null
+++ b/tests/fixtures/node-module-dynamic-import/package.json
@@ -0,0 +1,7 @@
+{
+  "name": "node-module-dynamic-import",
+  "version": "1.0.0",
+  "dependencies": {
+    "@org/test": "1.0.0"
+  }
+}
diff --git a/tests/main.js b/tests/main.js
index 3f3f02d..5591796 100644
--- a/tests/main.js
+++ b/tests/main.js
@@ -1063,6 +1063,24 @@ test('Adds `type: "functionsBundling"` to esbuild bundling errors', async (t) =>
   }
 })
 
+test('Returns a list of all modules with dynamic imports in a `nodeModulesWithDynamicImports` property', async (t) => {
+  const { files } = await zipNode(t, 'node-module-dynamic-import', {
+    opts: { config: { '*': { nodeBundler: ESBUILD } } },
+  })
+
+  t.is(files[0].nodeModulesWithDynamicImports.length, 2)
+  t.true(files[0].nodeModulesWithDynamicImports.includes('@org/test'))
+  t.true(files[0].nodeModulesWithDynamicImports.includes('test-two'))
+})
+
+test('Returns an empty list of modules with dynamic imports if the modules are missing a `package.json`', async (t) => {
+  const { files } = await zipNode(t, 'node-module-dynamic-import-invalid', {
+    opts: { config: { '*': { nodeBundler: ESBUILD } } },
+  })
+
+  t.is(files[0].nodeModulesWithDynamicImports.length, 0)
+})
+
 test('Uses the default Node bundler if no configuration object is supplied', async (t) => {
   const { files, tmpDir } = await zipNode(t, 'local-node-module')
   const requires = await getRequires({ filePath: resolve(tmpDir, 'src/function.js') })