Repository: netlify/zip-it-and-ship-it
Pull request #353 (2021-03-01 18:00:07)

fix: detect native bindings and change extension priority
**- Summary**

This PR changes the order of `resolveExtensions` to prefer `.js` files over `.mjs`. This fixes an issue with the `node-fetch` module. More information at https://github.com/evanw/esbuild/issues/363#issuecomment-787978404.

Additionally, it removes an [optimisation for external modules](https://github.com/netlify/zip-it-and-ship-it/commit/bdcf286fb83ee55f955585d0b3a9f7fe85600565#diff-dda86982a0cad48036d5871c2d2f251ac96f1b30089e1e4879cd9903eec8cbcbL73-L80) which, as @erezrokah pointed out, can introduce problems with different versions of the same module. It feels dangerous to be risking problems that are difficult to troubleshoot due to what may well be a premature optimisation.

Finally, it adds an esbuild plugin that detects Node bindings (via a direct `require` call to a `.node` file) and adds the corresponding file to the zip, avoiding a fallback to ZISI.

**- Test plan**

Added a test specifically for `node-fetch` and amended the test for modules with native bindings.

**- Description for the changelog**

fix: detect native bindings and change extension priority

**- A picture of a cute animal (not mandatory but encouraged)**

![hamster](https://user-images.githubusercontent.com/4162329/109538160-a887ad80-7ab7-11eb-94fa-312d9ddc8e12.jpg)



Patch:
diff --git a/src/bundler.js b/src/node_bundler/index.js
similarity index 65%
rename from src/bundler.js
rename to src/node_bundler/index.js
index 5f5a9cf..dbff1b2 100644
--- a/src/bundler.js
+++ b/src/node_bundler/index.js
@@ -6,10 +6,13 @@ const { promisify } = require('util')
 const esbuild = require('esbuild')
 const semver = require('semver')
 
+const { getPlugins } = require('./plugins')
+
 const pUnlink = promisify(fs.unlink)
 
 const bundleJsFile = async function ({
   additionalModulePaths,
+  basePath,
   destFilename,
   destFolder,
   externalModules = [],
@@ -20,13 +23,19 @@ const bundleJsFile = async function ({
   const external = [...new Set([...externalModules, ...ignoredModules])]
   const jsFilename = `${basename(destFilename, extname(destFilename))}.js`
   const bundlePath = join(destFolder, jsFilename)
+  const pluginContext = {
+    nodeBindings: new Set(),
+  }
 
   // esbuild's async build API throws on Node 8.x, so we switch to the sync
   // version for that version range.
-  const shouldUseAsyncAPI = semver.satisfies(process.version, '>=9.x')
+  const supportsAsyncAPI = semver.satisfies(process.version, '>=9.x')
+
+  // The sync API does not support plugins.
+  const plugins = supportsAsyncAPI ? getPlugins({ additionalModulePaths, basePath, context: pluginContext }) : undefined
 
   // eslint-disable-next-line node/no-sync
-  const buildFunction = shouldUseAsyncAPI ? esbuild.build : esbuild.buildSync
+  const buildFunction = supportsAsyncAPI ? esbuild.build : esbuild.buildSync
   const data = await buildFunction({
     bundle: true,
     entryPoints: [srcFile],
@@ -35,6 +44,8 @@ const bundleJsFile = async function ({
     outfile: bundlePath,
     nodePaths: additionalModulePaths,
     platform: 'node',
+    plugins,
+    resolveExtensions: ['.js', '.jsx', '.mjs', '.cjs', '.json'],
     target: ['es2017'],
   })
   const cleanTempFiles = async () => {
@@ -44,8 +55,9 @@ const bundleJsFile = async function ({
       // no-op
     }
   }
+  const additionalSrcFiles = [...pluginContext.nodeBindings]
 
-  return { bundlePath, cleanTempFiles, data }
+  return { bundlePath, cleanTempFiles, data: { ...data, additionalSrcFiles } }
 }
 
 module.exports = { bundleJsFile }
diff --git a/src/node_bundler/plugins.js b/src/node_bundler/plugins.js
new file mode 100644
index 0000000..5559b63
--- /dev/null
+++ b/src/node_bundler/plugins.js
@@ -0,0 +1,22 @@
+const { relative, resolve } = require('path')
+
+const getNodeBindingHandlerPlugin = ({ basePath, context }) => ({
+  name: 'node-binding-handler',
+  setup(build) {
+    build.onResolve({ filter: /\.node$/ }, (args) => {
+      const fullPath = resolve(args.resolveDir, args.path)
+      const resolvedPath = relative(basePath, fullPath)
+
+      context.nodeBindings.add(fullPath)
+
+      return {
+        external: true,
+        path: resolvedPath,
+      }
+    })
+  },
+})
+
+const getPlugins = ({ basePath, context }) => [getNodeBindingHandlerPlugin({ basePath, context })]
+
+module.exports = { getPlugins }
diff --git a/src/runtimes/node.js b/src/runtimes/node.js
index 89fe8ed..9079cfe 100644
--- a/src/runtimes/node.js
+++ b/src/runtimes/node.js
@@ -2,7 +2,7 @@ const { basename, dirname, join, normalize } = require('path')
 
 const commonPathPrefix = require('common-path-prefix')
 
-const { bundleJsFile } = require('../bundler')
+const { bundleJsFile } = require('../node_bundler')
 const {
   getDependencyNamesAndPathsForDependencies,
   getExternalAndIgnoredModulesFromSpecialCases,
@@ -51,6 +51,7 @@ const getSrcFilesAndExternalModules = async function ({
   }
 }
 
+// eslint-disable-next-line max-statements
 const zipFunction = async function ({
   destFolder,
   extension,
@@ -69,16 +70,8 @@ const zipFunction = async function ({
     externalModules: externalModulesFromSpecialCases,
     ignoredModules: ignoredModulesFromSpecialCases,
   } = await getExternalAndIgnoredModulesFromSpecialCases({ srcDir })
-
-  // When a module is added to `externalModules`, we will traverse its main
-  // file recursively and look for all its dependencies, so that we can ship
-  // their files separately, inside a `node_modules` directory. Whenever we
-  // process a module this way, we can also flag it as external with esbuild
-  // since its source is already part of the artifact and there's no point in
-  // inlining it again in the bundle.
-  // As such, the dependency traversal logic will compile the names of these
-  // modules in `additionalExternalModules`.
-  const { moduleNames: externalModulesFromTraversal = [], paths: srcFiles } = await getSrcFilesAndExternalModules({
+  const externalModules = [...new Set([...externalModulesFromConfig, ...externalModulesFromSpecialCases])]
+  const { paths: srcFiles } = await getSrcFilesAndExternalModules({
     stat,
     mainFile,
     extension,
@@ -86,11 +79,12 @@ const zipFunction = async function ({
     srcDir,
     pluginsModulesPath,
     jsBundler,
-    jsExternalModules: [...new Set([...externalModulesFromConfig, ...externalModulesFromSpecialCases])],
+    jsExternalModules: externalModules,
   })
-  const dirnames = srcFiles.map((filePath) => normalize(dirname(filePath)))
 
   if (jsBundler === JS_BUNDLER_ZISI) {
+    const dirnames = srcFiles.map((filePath) => normalize(dirname(filePath)))
+
     await zipNodeJs({
       basePath: commonPathPrefix(dirnames),
       destFolder,
@@ -104,16 +98,15 @@ const zipFunction = async function ({
     return { bundler: JS_BUNDLER_ZISI, path: destPath }
   }
 
+  const mainFilePath = dirname(mainFile)
   const { bundlePath, data, cleanTempFiles } = await bundleJsFile({
     additionalModulePaths: pluginsModulesPath ? [pluginsModulesPath] : [],
+    basePath: mainFilePath,
     destFilename: filename,
     destFolder,
-    externalModules: [
-      ...externalModulesFromConfig,
-      ...externalModulesFromSpecialCases,
-      ...externalModulesFromTraversal,
-    ],
+    externalModules,
     ignoredModules: [...ignoredModulesFromConfig, ...ignoredModulesFromSpecialCases],
+    srcDir,
     srcFile: mainFile,
   })
   const bundlerWarnings = data.warnings.length === 0 ? undefined : data.warnings
@@ -123,7 +116,9 @@ const zipFunction = async function ({
   const aliases = {
     [mainFile]: bundlePath,
   }
-  const basePath = commonPathPrefix([...dirnames, dirname(mainFile)])
+  const srcFilesAfterBundling = [...srcFiles, ...data.additionalSrcFiles]
+  const dirnames = srcFilesAfterBundling.map((filePath) => normalize(dirname(filePath)))
+  const basePath = commonPathPrefix([...dirnames, mainFilePath])
 
   try {
     await zipNodeJs({
@@ -134,7 +129,7 @@ const zipFunction = async function ({
       filename,
       mainFile,
       pluginsModulesPath,
-      srcFiles,
+      srcFiles: srcFilesAfterBundling,
     })
   } finally {
     await cleanTempFiles()


Test patch:
diff --git a/tests/fixtures/node-fetch/function.js b/tests/fixtures/node-fetch/function.js
new file mode 100644
index 0000000..f23840f
--- /dev/null
+++ b/tests/fixtures/node-fetch/function.js
@@ -0,0 +1,3 @@
+const fetch = require('node-fetch')
+
+module.exports = fetch
diff --git a/tests/fixtures/node-fetch/node_modules/node-fetch/lib/index.js b/tests/fixtures/node-fetch/node_modules/node-fetch/lib/index.js
new file mode 100644
index 0000000..5b48934
--- /dev/null
+++ b/tests/fixtures/node-fetch/node_modules/node-fetch/lib/index.js
@@ -0,0 +1,4 @@
+function fetch() {}
+
+module.exports = fetch
+module.exports.default = fetch
diff --git a/tests/fixtures/node-fetch/node_modules/node-fetch/lib/index.mjs b/tests/fixtures/node-fetch/node_modules/node-fetch/lib/index.mjs
new file mode 100644
index 0000000..4a81b49
--- /dev/null
+++ b/tests/fixtures/node-fetch/node_modules/node-fetch/lib/index.mjs
@@ -0,0 +1,3 @@
+function fetch() {}
+
+export default fetch
diff --git a/tests/fixtures/node-fetch/node_modules/node-fetch/package.json b/tests/fixtures/node-fetch/node_modules/node-fetch/package.json
new file mode 100644
index 0000000..1ae76ea
--- /dev/null
+++ b/tests/fixtures/node-fetch/node_modules/node-fetch/package.json
@@ -0,0 +1,6 @@
+{
+  "name": "node-fetch",
+  "version": "2.6.1",
+  "main": "lib/index",
+  "module": "lib/index.mjs"
+}
diff --git a/tests/fixtures/node-fetch/package.json b/tests/fixtures/node-fetch/package.json
new file mode 100644
index 0000000..16ae22e
--- /dev/null
+++ b/tests/fixtures/node-fetch/package.json
@@ -0,0 +1,7 @@
+{
+  "name": "function-requiring-node-fetch",
+  "version": "1.0.0",
+  "dependencies": {
+    "node-fetch": "^2.6.1"
+  }
+}
diff --git a/tests/main.js b/tests/main.js
index 649da31..1c6882c 100644
--- a/tests/main.js
+++ b/tests/main.js
@@ -1,7 +1,7 @@
 const { readFile, chmod, symlink, unlink, rename } = require('fs')
 const { tmpdir } = require('os')
 const { normalize, resolve } = require('path')
-const { platform } = require('process')
+const { platform, version } = require('process')
 const { promisify } = require('util')
 
 const test = require('ava')
@@ -9,7 +9,9 @@ const cpy = require('cpy')
 const del = require('del')
 const execa = require('execa')
 const pathExists = require('path-exists')
+const semver = require('semver')
 const { dir: getTmpDir, tmpName } = require('tmp-promise')
+const unixify = require('unixify')
 
 const { zipFunction, listFunctions, listFunctionsFiles } = require('..')
 const { JS_BUNDLER_ESBUILD: ESBUILD, JS_BUNDLER_ESBUILD_ZISI: ESBUILD_ZISI } = require('../src/utils/consts')
@@ -24,6 +26,8 @@ const pSymlink = promisify(symlink)
 const pUnlink = promisify(unlink)
 const pRename = promisify(rename)
 
+const supportsEsbuildPlugins = semver.satisfies(version, '>=9.x')
+
 // Alias for the default bundler.
 const DEFAULT = undefined
 const EXECUTABLE_PERMISSION = 0o755
@@ -54,13 +58,29 @@ testBundlers('Zips Node.js function files', [ESBUILD, ESBUILD_ZISI, DEFAULT], as
   t.true(files.every(({ runtime }) => runtime === 'js'))
 })
 
-testBundlers('Handles Node module with native bindings', [ESBUILD, ESBUILD_ZISI, DEFAULT], async (bundler, t) => {
-  const jsExternalModules = bundler === ESBUILD || bundler === ESBUILD ? ['test'] : undefined
-  const { files } = await zipNode(t, 'node-module-native', {
-    opts: { jsBundler: bundler, jsExternalModules },
-  })
-  t.true(files.every(({ runtime }) => runtime === 'js'))
-})
+// Bundling modules with native bindings with esbuild is expected to fail on
+// Node 8 because esbuild plugins are not supported. The ZISI fallback should
+// kick in.
+testBundlers(
+  'Handles Node module with native bindings',
+  [ESBUILD_ZISI, DEFAULT, supportsEsbuildPlugins && ESBUILD].filter(Boolean),
+  async (bundler, t) => {
+    const { files, tmpDir } = await zipNode(t, 'node-module-native', {
+      opts: { jsBundler: bundler },
+    })
+    const requires = await getRequires({ filePath: resolve(tmpDir, 'src/function.js') })
+    const normalizedRequires = new Set(requires.map(unixify))
+
+    t.true(files.every(({ runtime }) => runtime === 'js'))
+    t.true(await pathExists(`${tmpDir}/src/node_modules/test/native.node`))
+
+    if (files[0].bundler === 'esbuild') {
+      t.true(normalizedRequires.has('node_modules/test/native.node'))
+    } else {
+      t.true(normalizedRequires.has('test'))
+    }
+  },
+)
 
 testBundlers('Can require node modules', [ESBUILD, ESBUILD_ZISI, DEFAULT], async (bundler, t) => {
   await zipNode(t, 'local-node-module', { opts: { jsBundler: bundler } })
@@ -557,6 +577,17 @@ testBundlers(
   },
 )
 
+testBundlers(
+  'Exposes the main export of `node-fetch` when imported using `require()`',
+  [ESBUILD, ESBUILD_ZISI, DEFAULT],
+  async (bundler, t) => {
+    const { files, tmpDir } = await zipFixture(t, 'node-fetch', { opts: { jsBundler: bundler } })
+    await unzipFiles(files)
+    // eslint-disable-next-line import/no-dynamic-require, node/global-require
+    t.true(typeof require(`${tmpDir}/function.js`) === 'function')
+  },
+)
+
 test('Zips Rust function files', async (t) => {
   const { files, tmpDir } = await zipFixture(t, 'rust-simple', { length: 1 })