Repository: netlify/zip-it-and-ship-it (js)

Issue:
Limit `nodeModulesWithDynamicImports` to unresolved dynamic imports
**- What is the current behavior?**

When bundling Node functions with esbuild, zip-it-and-ship-it will return a `nodeModulesWithDynamicImports` property, which contains a list of modules that make use of `require`/`import` statements with dynamic expressions. This allows consumers such as Netlify Build to warn users of potential issues and suggest workarounds.

_Example:_

```
4:41:05 PM: ❯ The following Node.js modules use dynamic expressions to include files:
4:41:05 PM:    - next
4:41:05 PM:    - @ampproject/toolbox-optimizer
4:41:05 PM: 
4:41:05 PM:   Because files included with dynamic expressions aren't bundled with your serverless functions by default,
  this may result in an error when invoking a function. To resolve this error, you can mark these Node.js
4:41:05 PM:   modules as external in the [functions] section of your `netlify.toml` configuration file:
4:41:05 PM: 
4:41:05 PM:   [functions]
4:41:05 PM:     external_node_modules = ["next", "@ampproject/toolbox-optimizer"]
4:41:05 PM: 
4:41:05 PM:   Visit https://ntl.fyi/dynamic-imports for more information.
```

We added this warning at a time when we weren't able to do anything about these imports, so flagging any potential issues was our best option. Now, we're actually capable of handling some of these imports, so this warning becomes misleading.

**- What is the expected behavior?**

We should only include in this warning modules with dynamic imports that we weren't able to solve.

From a quick look, we should be able to achieve this by running [this call](https://github.com/netlify/zip-it-and-ship-it/blob/main/src/runtimes/node/dynamic_imports/plugin.js#L25) only when exiting early, i.e. when we can't process the import.

Pull request description:
fix: limit `nodeModulesWithDynamicImports` to unresolved imports
**- Summary**

Only adds modules to `nodeModulesWithDynamicImports` when the imports can't be resolved. More details in the linked issue.

Closes #627

**- Test plan**

Adjusted existing test

**- A picture of a cute animal (not mandatory but encouraged)**

![seal-pup-waving-norfolk-beach-3-5e09b65aaaa2b__700](https://user-images.githubusercontent.com/4162329/134645409-a5eeba29-8eda-417d-a496-e56fa76910cd.jpg)




Patch:
diff --git a/src/runtimes/node/dynamic_imports/plugin.js b/src/runtimes/node/dynamic_imports/plugin.js
index ceff3fe..d16bd1b 100644
--- a/src/runtimes/node/dynamic_imports/plugin.js
+++ b/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 })
     })
   },
 })


Test patch:
diff --git a/tests/fixtures/node-module-dynamic-import/function.js b/tests/fixtures/node-module-dynamic-import/function.js
index 41fa369..42d9392 100644
--- a/tests/fixtures/node-module-dynamic-import/function.js
+++ b/tests/fixtures/node-module-dynamic-import/function.js
@@ -1,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]
diff --git a/tests/fixtures/node-module-dynamic-import/node_modules/@org/test_2/files/one.json b/tests/fixtures/node-module-dynamic-import/node_modules/@org/test_2/files/one.json
new file mode 100644
index 0000000..34204b5
--- /dev/null
+++ b/tests/fixtures/node-module-dynamic-import/node_modules/@org/test_2/files/one.json
@@ -0,0 +1,1 @@
+{"bool": true}
diff --git a/tests/fixtures/node-module-dynamic-import/node_modules/@org/test_2/index.js b/tests/fixtures/node-module-dynamic-import/node_modules/@org/test_2/index.js
new file mode 100644
index 0000000..2907af3
--- /dev/null
+++ b/tests/fixtures/node-module-dynamic-import/node_modules/@org/test_2/index.js
@@ -0,0 +1,5 @@
+module.exports = (number) => {
+  const file = require(`./files/${number}.json`)
+
+  return file.bool
+}
diff --git a/tests/fixtures/node-module-dynamic-import/node_modules/@org/test_2/package.json b/tests/fixtures/node-module-dynamic-import/node_modules/@org/test_2/package.json
new file mode 100644
index 0000000..f29a23d
--- /dev/null
+++ b/tests/fixtures/node-module-dynamic-import/node_modules/@org/test_2/package.json
@@ -0,0 +1,7 @@
+{
+  "name": "@org/test_2",
+  "version": "1.0.0",
+  "dependencies": {
+    "test-two": "1.0.0"
+  }
+}
diff --git a/tests/fixtures/node-module-dynamic-import/node_modules/test-three/index.js b/tests/fixtures/node-module-dynamic-import/node_modules/test-three/index.js
new file mode 100644
index 0000000..812557c
--- /dev/null
+++ b/tests/fixtures/node-module-dynamic-import/node_modules/test-three/index.js
@@ -0,0 +1,5 @@
+module.exports = () => {
+  const file = "./one.json"
+
+  return require(file)
+}
diff --git a/tests/fixtures/node-module-dynamic-import/node_modules/test-three/one.json b/tests/fixtures/node-module-dynamic-import/node_modules/test-three/one.json
new file mode 100644
index 0000000..34204b5
--- /dev/null
+++ b/tests/fixtures/node-module-dynamic-import/node_modules/test-three/one.json
@@ -0,0 +1,1 @@
+{"bool": true}
diff --git a/tests/fixtures/node-module-dynamic-import/node_modules/test-three/package.json b/tests/fixtures/node-module-dynamic-import/node_modules/test-three/package.json
new file mode 100644
index 0000000..788fc2a
--- /dev/null
+++ b/tests/fixtures/node-module-dynamic-import/node_modules/test-three/package.json
@@ -0,0 +1,4 @@
+{
+  "name": "test-three",
+  "version": "1.0.0"
+}
diff --git a/tests/fixtures/node-module-dynamic-import/package.json b/tests/fixtures/node-module-dynamic-import/package.json
index c0b4a81..600eeda 100644
--- a/tests/fixtures/node-module-dynamic-import/package.json
+++ b/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"
   }
 }
diff --git a/tests/main.js b/tests/main.js
index 2bae8a0..7725a08 100644
--- a/tests/main.js
+++ b/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) => {