Repository: netlify/zip-it-and-ship-it
Pull request #338 (2021-02-18 14:02:15)

feat: add Next.js special case to esbuild bundler
**- Summary**

This PR adds special cases for the Next.js dependency quirks to the esbuild bundling mechanism.

It also handles the case where a module that isn't installed is included in `externalModules`. A corresponding test for this case is included.

**- Test plan**

Relevant tests were moved from being legacy-specific to the common test pool.

**- Description for the changelog**

feat: add Next.js special case to esbuild bundler

**- A picture of a cute animal (not mandatory but encouraged)**

![b7c32f70a50b163bda92b6bcf1956515](https://user-images.githubusercontent.com/4162329/108367478-6aee6f00-71f1-11eb-9209-110df248eeab.jpg)




Patch:
diff --git a/src/node_dependencies/index.js b/src/node_dependencies/index.js
index b0893ae..c98369a 100644
--- a/src/node_dependencies/index.js
+++ b/src/node_dependencies/index.js
@@ -5,6 +5,7 @@ const precinct = require('precinct')
 
 const { getPackageJson } = require('./package_json')
 const { resolvePathPreserveSymlinks } = require('./resolve')
+const { getExternalAndIgnoredModulesFromSpecialCases } = require('./special_cases')
 const {
   getDependencyPathsForDependency,
   getDependencyNamesAndPathsForDependencies,
@@ -117,5 +118,6 @@ module.exports = {
   getDependencyPathsForDependency,
   getDependencyNamesAndPathsForDependencies,
   getDependencyNamesAndPathsForDependency,
+  getExternalAndIgnoredModulesFromSpecialCases,
   listFilesUsingLegacyBundler,
 }
diff --git a/src/node_dependencies/special_cases.js b/src/node_dependencies/special_cases.js
new file mode 100644
index 0000000..e820469
--- /dev/null
+++ b/src/node_dependencies/special_cases.js
@@ -0,0 +1,33 @@
+const { getPackageJson } = require('./package_json')
+
+const getPackageJsonIfAvailable = async (srcDir) => {
+  try {
+    const packageJson = await getPackageJson(srcDir)
+
+    return packageJson
+  } catch (_) {
+    return {}
+  }
+}
+
+const getModulesForNextJs = ({ dependencies }) => {
+  const externalModules = dependencies.next ? ['critters', 'nanoid'] : []
+  const ignoredModules = []
+
+  return {
+    externalModules,
+    ignoredModules,
+  }
+}
+
+const getExternalAndIgnoredModulesFromSpecialCases = async ({ srcDir }) => {
+  const { dependencies = {} } = await getPackageJsonIfAvailable(srcDir)
+  const { externalModules, ignoredModules } = getModulesForNextJs({ dependencies })
+
+  return {
+    externalModules,
+    ignoredModules,
+  }
+}
+
+module.exports = { getExternalAndIgnoredModulesFromSpecialCases }
diff --git a/src/node_dependencies/traverse.js b/src/node_dependencies/traverse.js
index 09be870..293f6a2 100644
--- a/src/node_dependencies/traverse.js
+++ b/src/node_dependencies/traverse.js
@@ -2,6 +2,7 @@ const { dirname } = require('path')
 
 const { getModuleName } = require('./module')
 const { getNestedDependencies, handleModuleNotFound } = require('./nested')
+const { getPackageJson } = require('./package_json')
 const { getPublishedFiles } = require('./published')
 const { resolvePackage } = require('./resolve')
 const { getSideFiles } = require('./side_files')
@@ -39,9 +40,9 @@ const getDependencyNamesAndPathsForDependencies = async function ({
   dependencies: dependencyNames,
   basedir,
   state = getNewCache(),
-  packageJson,
   pluginsModulesPath,
 }) {
+  const packageJson = await getPackageJson(basedir)
   const dependencies = await Promise.all(
     dependencyNames.map((dependencyName) =>
       getDependencyNamesAndPathsForDependency({
@@ -53,8 +54,9 @@ const getDependencyNamesAndPathsForDependencies = async function ({
       }),
     ),
   )
-  const moduleNames = new Set(dependencies.flatMap((dependency) => [...dependency.moduleNames]))
-  const paths = new Set(dependencies.flatMap((dependency) => [...dependency.paths]))
+  const filteredDependencies = dependencies.filter(Boolean)
+  const moduleNames = new Set(filteredDependencies.flatMap((dependency) => [...dependency.moduleNames]))
+  const paths = new Set(filteredDependencies.flatMap((dependency) => [...dependency.paths]))
 
   return {
     moduleNames: [...moduleNames],
@@ -69,11 +71,22 @@ const getDependencyNamesAndPathsForDependency = async function ({
   packageJson,
   pluginsModulesPath,
 }) {
-  const paths = await getDependencyPathsForDependency({ dependency, basedir, state, packageJson, pluginsModulesPath })
+  try {
+    const paths = await getDependencyPathsForDependency({ dependency, basedir, state, packageJson, pluginsModulesPath })
 
-  return {
-    moduleNames: [...state.moduleNames],
-    paths,
+    return {
+      moduleNames: [...state.moduleNames],
+      paths,
+    }
+  } catch (error) {
+    if (error.code === 'MODULE_NOT_FOUND') {
+      return {
+        moduleNames: [],
+        paths: [],
+      }
+    }
+
+    throw error
   }
 }
 
diff --git a/src/runtimes/node.js b/src/runtimes/node.js
index 38bd0f3..17ccf6e 100644
--- a/src/runtimes/node.js
+++ b/src/runtimes/node.js
@@ -3,7 +3,11 @@ const { basename, dirname, join, normalize } = require('path')
 const commonPathPrefix = require('common-path-prefix')
 
 const { bundleJsFile } = require('../bundler')
-const { getDependencyNamesAndPathsForDependencies, listFilesUsingLegacyBundler } = require('../node_dependencies')
+const {
+  getDependencyNamesAndPathsForDependencies,
+  getExternalAndIgnoredModulesFromSpecialCases,
+  listFilesUsingLegacyBundler,
+} = require('../node_dependencies')
 const { zipNodeJs } = require('../zip_node')
 
 const getSrcFiles = async function (options) {
@@ -49,9 +53,9 @@ const getSrcFilesAndExternalModules = async function ({
 const zipFunction = async function ({
   destFolder,
   extension,
-  externalModules,
+  externalModules: externalModulesFromConfig,
   filename,
-  ignoredModules,
+  ignoredModules: ignoredModulesFromConfig,
   mainFile,
   pluginsModulesPath,
   srcDir,
@@ -69,7 +73,7 @@ const zipFunction = async function ({
   // inlining it again in the bundle.
   // As such, the dependency traversal logic will compile the names of these
   // modules in `additionalExternalModules`.
-  const { moduleNames: additionalExternalModules = [], paths: srcFiles } = await getSrcFilesAndExternalModules({
+  const { moduleNames: externalModulesFromTraversal = [], paths: srcFiles } = await getSrcFilesAndExternalModules({
     stat,
     mainFile,
     extension,
@@ -77,7 +81,7 @@ const zipFunction = async function ({
     srcDir,
     pluginsModulesPath,
     useEsbuild,
-    externalModules,
+    externalModules: externalModulesFromConfig,
   })
   const dirnames = srcFiles.map((filePath) => normalize(dirname(filePath)))
 
@@ -95,12 +99,21 @@ const zipFunction = async function ({
     return destPath
   }
 
+  const {
+    externalModules: externalModulesFromSpecialCases,
+    ignoredModules: ignoredModulesFromSpecialCases,
+  } = await getExternalAndIgnoredModulesFromSpecialCases({ srcDir })
+
   const { bundlePath, cleanTempFiles } = await bundleJsFile({
     additionalModulePaths: pluginsModulesPath ? [pluginsModulesPath] : [],
     destFilename: filename,
     destFolder,
-    externalModules: [...externalModules, ...additionalExternalModules],
-    ignoredModules,
+    externalModules: [
+      ...externalModulesFromConfig,
+      ...externalModulesFromSpecialCases,
+      ...externalModulesFromTraversal,
+    ],
+    ignoredModules: [...ignoredModulesFromConfig, ...ignoredModulesFromSpecialCases],
     srcFile: mainFile,
   })
 


Test patch:
diff --git a/tests/main.js b/tests/main.js
index a444b05..a96c6ea 100644
--- a/tests/main.js
+++ b/tests/main.js
@@ -162,6 +162,22 @@ BUNDLERS.forEach((bundler) => {
     await zipNodeWithBundler(t, 'local-parent-require')
   })
 
+  test(`[bundler: ${bundler}] Ignore missing critters dependency for Next.js 10`, async (t) => {
+    await zipNodeWithBundler(t, 'node-module-next10-critters')
+  })
+
+  test(`[bundler: ${bundler}] Ignore missing critters dependency for Next.js exact version 10.0.5`, async (t) => {
+    await zipNodeWithBundler(t, 'node-module-next10-critters-exact')
+  })
+
+  test(`[bundler: ${bundler}] Ignore missing critters dependency for Next.js with range ^10.0.5`, async (t) => {
+    await zipNodeWithBundler(t, 'node-module-next10-critters-10.0.5-range')
+  })
+
+  test(`[bundler: ${bundler}] Ignore missing critters dependency for Next.js with version='latest'`, async (t) => {
+    await zipNodeWithBundler(t, 'node-module-next10-critters-latest')
+  })
+
   // Need to create symlinks dynamically because they sometimes get lost when
   // committed on Windows
   if (platform !== 'win32') {
@@ -408,20 +424,32 @@ test('[bundler: legacy] Throws on missing critters dependency for Next.js 9', as
   await t.throwsAsync(zipNode(t, 'node-module-next9-critters', { bundler: 'legacy' }))
 })
 
-test('[bundler: legacy] Ignore missing critters dependency for Next.js 10', async (t) => {
-  await zipNode(t, 'node-module-next10-critters', { bundler: 'legacy' })
-})
-
-test('[bundler: legacy] Ignore missing critters dependency for Next.js exact version 10.0.5', async (t) => {
-  await zipNode(t, 'node-module-next10-critters-exact', { bundler: 'legacy' })
-})
-
-test('[bundler: legacy] Ignore missing critters dependency for Next.js with range ^10.0.5', async (t) => {
-  await zipNode(t, 'node-module-next10-critters-10.0.5-range', { bundler: 'legacy' })
+test('[bundler: legacy] Includes specific Next.js dependencies when using next-on-netlify', async (t) => {
+  const { tmpDir } = await zipNode(t, 'node-module-next-on-netlify', { bundler: 'legacy' })
+  const [constantsExists, semverExists, otherExists, indexExists] = await Promise.all([
+    pathExists(`${tmpDir}/src/node_modules/next/dist/next-server/lib/constants.js`),
+    pathExists(`${tmpDir}/src/node_modules/next/dist/compiled/semver.js`),
+    pathExists(`${tmpDir}/src/node_modules/next/dist/other.js`),
+    pathExists(`${tmpDir}/src/node_modules/next/index.js`),
+  ])
+  t.true(constantsExists)
+  t.true(semverExists)
+  t.false(otherExists)
+  t.false(indexExists)
 })
 
-test("[bundler: legacy] Ignore missing critters dependency for Next.js with version='latest'", async (t) => {
-  await zipNode(t, 'node-module-next10-critters-latest', { bundler: 'legacy' })
+test('[bundler: legacy] Includes all Next.js dependencies when not using next-on-netlify', async (t) => {
+  const { tmpDir } = await zipNode(t, 'node-module-next', { bundler: 'legacy' })
+  const [constantsExists, semverExists, otherExists, indexExists] = await Promise.all([
+    pathExists(`${tmpDir}/src/node_modules/next/dist/next-server/lib/constants.js`),
+    pathExists(`${tmpDir}/src/node_modules/next/dist/compiled/semver.js`),
+    pathExists(`${tmpDir}/src/node_modules/next/dist/other.js`),
+    pathExists(`${tmpDir}/src/node_modules/next/index.js`),
+  ])
+  t.true(constantsExists)
+  t.true(semverExists)
+  t.true(otherExists)
+  t.true(indexExists)
 })
 
 // esbuild bundler tests.
@@ -468,30 +496,11 @@ test('[bundler: esbuild] Include most files from node modules present in `extern
   t.true(htmlExists)
 })
 
-test('[bundler: legacy] Includes specific Next.js dependencies when using next-on-netlify', async (t) => {
-  const { tmpDir } = await zipNode(t, 'node-module-next-on-netlify', { bundler: 'legacy' })
-  const [constantsExists, semverExists, otherExists, indexExists] = await Promise.all([
-    pathExists(`${tmpDir}/src/node_modules/next/dist/next-server/lib/constants.js`),
-    pathExists(`${tmpDir}/src/node_modules/next/dist/compiled/semver.js`),
-    pathExists(`${tmpDir}/src/node_modules/next/dist/other.js`),
-    pathExists(`${tmpDir}/src/node_modules/next/index.js`),
-  ])
-  t.true(constantsExists)
-  t.true(semverExists)
-  t.false(otherExists)
-  t.false(indexExists)
-})
+test('[bundler: esbuild] Does not throw if one of the modules defined in `externalModules` does not exist', async (t) => {
+  const { tmpDir } = await zipNode(t, 'node-module-included-try-catch', {
+    bundler: 'esbuild',
+    opts: { externalModules: ['i-do-not-exist'] },
+  })
 
-test('[bundler: legacy] Includes all Next.js dependencies when not using next-on-netlify', async (t) => {
-  const { tmpDir } = await zipNode(t, 'node-module-next', { bundler: 'legacy' })
-  const [constantsExists, semverExists, otherExists, indexExists] = await Promise.all([
-    pathExists(`${tmpDir}/src/node_modules/next/dist/next-server/lib/constants.js`),
-    pathExists(`${tmpDir}/src/node_modules/next/dist/compiled/semver.js`),
-    pathExists(`${tmpDir}/src/node_modules/next/dist/other.js`),
-    pathExists(`${tmpDir}/src/node_modules/next/index.js`),
-  ])
-  t.true(constantsExists)
-  t.true(semverExists)
-  t.true(otherExists)
-  t.true(indexExists)
+  t.false(await pathExists(`${tmpDir}/src/node_modules/i-do-not-exist`))
 })