fix: limit nodeModulesWithDynamicImports to unresolved imports (net…

…lify/zip-it-and-ship-it#671)

* feat: limit nodeModulesWithDynamicImports to unresolved imports

* chore: update test

Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>

packages/zip-it-and-ship-it/src/runtimes/node/dynamic_imports/plugin.js

@@ -22,36 +22,35 @@ const getDynamicImportsPlugin = ({ basePath, includedPaths, moduleNames, process
     build.onDynamicImport({}, async (args) => {
       const { expression, resolveDir } = args
 
-      await registerModuleWithDynamicImports({ cache, moduleNames, resolveDir, srcDir })
-
       // Don't attempt to parse the expression if the base path isn't defined,
       // since we won't be able to generate the globs for the included paths.
       // Also don't parse the expression if we're not interested in processing
       // the dynamic import expressions.
-      if (!basePath || !processImports) {
-        return
-      }
-
-      const { includedPathsGlob, type: expressionType } = parseExpression({ basePath, expression, resolveDir }) || {}
-
-      if (!includedPathsGlob) {
-        return
+      if (basePath && processImports) {
+        const { includedPathsGlob, type: expressionType } = parseExpression({ basePath, expression, resolveDir }) || {}
+
+        if (includedPathsGlob) {
+          // The parser has found a glob of paths that should be included in the
+          // bundle to make this import work, so we add it to `includedPaths`.
+          includedPaths.add(includedPathsGlob)
+
+          // Create the shim that will handle the import at runtime.
+          const contents = getShimContents({ expressionType, resolveDir, srcDir })
+
+          // This is the only branch where we actually solve a dynamic import.
+          // eslint-disable-next-line max-depth
+          if (contents) {
+            return {
+              contents,
+            }
+          }
+        }
       }
 
-      // The parser has found a glob of paths that should be included in the
-      // bundle to make this import work, so we add it to `includedPaths`.
-      includedPaths.add(includedPathsGlob)
-
-      // Create the shim that will handle the import at runtime.
-      const contents = getShimContents({ expressionType, resolveDir, srcDir })
-
-      if (!contents) {
-        return
-      }
-
-      return {
-        contents,
-      }
+      // If we're here, it means we weren't able to solve the dynamic import.
+      // We add it to the list of modules with dynamic imports, which allows
+      // consumers like Netlify Build or CLI to advise users on how to proceed.
+      await registerModuleWithDynamicImports({ cache, moduleNames, resolveDir, srcDir })
     })
   },
 })

packages/zip-it-and-ship-it/tests/fixtures/node-module-dynamic-import/function.js

@@ -1 +1,5 @@
-module.exports = require('@org/test')
+const test1 = require('@org/test')
+const test2 = require('@org/test_2')
+const test3 = require('test-three')
+
+module.exports = [test1, test2, test3]

packages/zip-it-and-ship-it/tests/fixtures/node-module-dynamic-import/package.json

@@ -2,6 +2,8 @@
   "name": "node-module-dynamic-import",
   "version": "1.0.0",
   "dependencies": {
-    "@org/test": "1.0.0"
+    "@org/test": "1.0.0",
+    "@org/test_2": "1.0.0",
+    "test-three": "1.0.0"
   }
 }

packages/zip-it-and-ship-it/tests/main.js

@@ -1455,13 +1455,14 @@ 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 } } },
+  const fixtureName = 'node-module-dynamic-import'
+  const { files } = await zipNode(t, fixtureName, {
+    opts: { basePath: join(FIXTURES_DIR, fixtureName), 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'))
+  t.true(files[0].nodeModulesWithDynamicImports.includes('test-three'))
 })
 
 test('Returns an empty list of modules with dynamic imports if the modules are missing a `package.json`', async (t) => {
@@ -1508,8 +1509,7 @@ test('Adds a runtime shim and includes the files needed for dynamic imports usin
   // eslint-disable-next-line unicorn/new-for-builtins
   t.deepEqual(values, Array(expectedLength).fill(true))
   t.throws(() => func('two'))
-  t.is(files[0].nodeModulesWithDynamicImports.length, 1)
-  t.true(files[0].nodeModulesWithDynamicImports.includes('@org/test'))
+  t.is(files[0].nodeModulesWithDynamicImports.length, 0)
 })
 
 test('Leaves dynamic imports untouched when the files required to resolve the expression cannot be packaged at build time', async (t) => {