From bdcf286fb83ee55f955585d0b3a9f7fe85600565 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eduardo=20Bou=C3=A7as?= Date: Mon, 1 Mar 2021 14:45:26 +0000 Subject: [PATCH 1/7] fix: remove externalModules optimisation --- src/runtimes/node.js | 35 ++++++++++------------------------- 1 file changed, 10 insertions(+), 25 deletions(-) diff --git a/src/runtimes/node.js b/src/runtimes/node.js index 89fe8edfb..1554ca01c 100644 --- a/src/runtimes/node.js +++ b/src/runtimes/node.js @@ -69,16 +69,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,7 +78,7 @@ const zipFunction = async function ({ srcDir, pluginsModulesPath, jsBundler, - jsExternalModules: [...new Set([...externalModulesFromConfig, ...externalModulesFromSpecialCases])], + jsExternalModules: externalModules, }) const dirnames = srcFiles.map((filePath) => normalize(dirname(filePath))) @@ -108,27 +100,20 @@ const zipFunction = async function ({ additionalModulePaths: pluginsModulesPath ? [pluginsModulesPath] : [], destFilename: filename, destFolder, - externalModules: [ - ...externalModulesFromConfig, - ...externalModulesFromSpecialCases, - ...externalModulesFromTraversal, - ], + externalModules, ignoredModules: [...ignoredModulesFromConfig, ...ignoredModulesFromSpecialCases], srcFile: mainFile, }) const bundlerWarnings = data.warnings.length === 0 ? undefined : data.warnings - // We're adding the bundled file to the zip, but we want it to have the same - // name and path as the original, unbundled file. For this, we use an alias.. - const aliases = { - [mainFile]: bundlePath, - } - const basePath = commonPathPrefix([...dirnames, dirname(mainFile)]) - try { await zipNodeJs({ - aliases, - basePath, + // We're adding the bundled file to the zip, but we want it to have the same + // name and path as the original, unbundled file. For this, we use an alias.. + aliases: { + [mainFile]: bundlePath, + }, + basePath: commonPathPrefix([...dirnames, dirname(mainFile)]), destFolder, destPath, filename, From 174a38cadd5bd648f4ed904ce6afce6aa6e8f760 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eduardo=20Bou=C3=A7as?= Date: Mon, 1 Mar 2021 15:50:29 +0000 Subject: [PATCH 2/7] fix: add esbuild plugin for node-fetch --- src/{bundler.js => node_bundler/index.js} | 3 + src/node_bundler/plugins.js | 58 +++++++++++++++++++ src/runtimes/node.js | 2 +- .../node_modules/node-fetch/lib/index.js | 4 ++ .../node_modules/node-fetch/lib/index.mjs | 3 + .../node_modules/node-fetch/package.json | 6 ++ tests/fixtures/node-fetch/function.js | 4 ++ tests/fixtures/node-fetch/package.json | 7 +++ tests/main.js | 11 ++++ 9 files changed, 97 insertions(+), 1 deletion(-) rename src/{bundler.js => node_bundler/index.js} (93%) create mode 100644 src/node_bundler/plugins.js create mode 100644 tests/fixtures/node-fetch/.netlify/plugins/node_modules/node-fetch/lib/index.js create mode 100644 tests/fixtures/node-fetch/.netlify/plugins/node_modules/node-fetch/lib/index.mjs create mode 100644 tests/fixtures/node-fetch/.netlify/plugins/node_modules/node-fetch/package.json create mode 100644 tests/fixtures/node-fetch/function.js create mode 100644 tests/fixtures/node-fetch/package.json diff --git a/src/bundler.js b/src/node_bundler/index.js similarity index 93% rename from src/bundler.js rename to src/node_bundler/index.js index 5f5a9cf04..cad95aceb 100644 --- a/src/bundler.js +++ b/src/node_bundler/index.js @@ -6,6 +6,8 @@ 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 ({ @@ -35,6 +37,7 @@ const bundleJsFile = async function ({ outfile: bundlePath, nodePaths: additionalModulePaths, platform: 'node', + plugins: getPlugins({ additionalModulePaths }), target: ['es2017'], }) const cleanTempFiles = async () => { diff --git a/src/node_bundler/plugins.js b/src/node_bundler/plugins.js new file mode 100644 index 000000000..96ec92859 --- /dev/null +++ b/src/node_bundler/plugins.js @@ -0,0 +1,58 @@ +// This plugin addresses an edge case with `node-fetch`. The module's `main` +// export points to an ambiguous path (`lib/index`) which, by default, is +// resolved to an ESM file by esbuild. This plugin hijacks `require()` calls +// to the module and forces the path to be `lib/index.js`. +const getNodeFetchHandlerPlugin = ({ additionalModulePaths }) => ({ + name: 'node-fetch-handler', + setup(build) { + build.onResolve({ filter: /^node-fetch$/ }, ({ kind, path }) => { + if (kind !== 'require-call') { + return + } + + if (additionalModulePaths.length === 0) { + return { + path: require.resolve(path), + } + } + + // If we have additional module paths, these need to be included in the + // `require.resolve` call. Unfortunately, the `options.path` parameter + // doesn't allow us to simply add a path to the ones it uses by default. + // We either use the defaults or we have to specify all the paths. This + // can be done by merging `require.resolve.paths` with the paths we want + // to add, but that primitive is not available in Node 8. In that case, + // we re-try the `require.resolve` call with the additional paths if it + // fails initially. + + // It's safe to disable this rule because we're checking whether the + // function exists before attempting to use it. + // + // @todo Remove once we drop support for Node 8. + // eslint-disable-next-line node/no-unsupported-features/node-builtins + const getDefaultRequirePaths = require.resolve.paths + + if (typeof getDefaultRequirePaths !== 'function') { + try { + return { + path: require.resolve(path), + } + } catch (_) { + return { + path: require.resolve(path, { paths: additionalModulePaths }), + } + } + } + + const paths = [...getDefaultRequirePaths(path), ...additionalModulePaths] + + return { + path: require.resolve(path, { paths }), + } + }) + }, +}) + +const getPlugins = ({ additionalModulePaths }) => [getNodeFetchHandlerPlugin({ additionalModulePaths })] + +module.exports = { getPlugins } diff --git a/src/runtimes/node.js b/src/runtimes/node.js index 1554ca01c..c7121f9f3 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, diff --git a/tests/fixtures/node-fetch/.netlify/plugins/node_modules/node-fetch/lib/index.js b/tests/fixtures/node-fetch/.netlify/plugins/node_modules/node-fetch/lib/index.js new file mode 100644 index 000000000..5b4893418 --- /dev/null +++ b/tests/fixtures/node-fetch/.netlify/plugins/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/.netlify/plugins/node_modules/node-fetch/lib/index.mjs b/tests/fixtures/node-fetch/.netlify/plugins/node_modules/node-fetch/lib/index.mjs new file mode 100644 index 000000000..4a81b49a7 --- /dev/null +++ b/tests/fixtures/node-fetch/.netlify/plugins/node_modules/node-fetch/lib/index.mjs @@ -0,0 +1,3 @@ +function fetch() {} + +export default fetch diff --git a/tests/fixtures/node-fetch/.netlify/plugins/node_modules/node-fetch/package.json b/tests/fixtures/node-fetch/.netlify/plugins/node_modules/node-fetch/package.json new file mode 100644 index 000000000..1ae76ea23 --- /dev/null +++ b/tests/fixtures/node-fetch/.netlify/plugins/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/function.js b/tests/fixtures/node-fetch/function.js new file mode 100644 index 000000000..e72ccfdc1 --- /dev/null +++ b/tests/fixtures/node-fetch/function.js @@ -0,0 +1,4 @@ +// eslint-disable-next-line import/no-unresolved +const fetch = require('node-fetch') + +module.exports = fetch diff --git a/tests/fixtures/node-fetch/package.json b/tests/fixtures/node-fetch/package.json new file mode 100644 index 000000000..16ae22ebe --- /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 649da31e6..00dbacfa8 100644 --- a/tests/main.js +++ b/tests/main.js @@ -557,6 +557,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 }) From 508ea0022a13377ab54dab2b272babe42a47a305 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eduardo=20Bou=C3=A7as?= Date: Mon, 1 Mar 2021 18:21:36 +0000 Subject: [PATCH 3/7] chore: skip esbuild plugins in Node 8 --- src/node_bundler/index.js | 9 ++++++--- tests/main.js | 8 ++++++-- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/node_bundler/index.js b/src/node_bundler/index.js index cad95aceb..ba59b200f 100644 --- a/src/node_bundler/index.js +++ b/src/node_bundler/index.js @@ -25,10 +25,13 @@ const bundleJsFile = async function ({ // 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 }) : 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], @@ -37,7 +40,7 @@ const bundleJsFile = async function ({ outfile: bundlePath, nodePaths: additionalModulePaths, platform: 'node', - plugins: getPlugins({ additionalModulePaths }), + plugins, target: ['es2017'], }) const cleanTempFiles = async () => { diff --git a/tests/main.js b/tests/main.js index 00dbacfa8..556109aa2 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,6 +9,7 @@ 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 { zipFunction, listFunctions, listFunctionsFiles } = require('..') @@ -557,9 +558,12 @@ testBundlers( }, ) +// This test relies on esbuild plugins, which aren't supported in Node 8. +const nodeFetchBundlers = semver.satisfies(version, '>=9.x') ? [ESBUILD, ESBUILD_ZISI, DEFAULT] : [DEFAULT] + testBundlers( 'Exposes the main export of `node-fetch` when imported using `require()`', - [ESBUILD, ESBUILD_ZISI, DEFAULT], + nodeFetchBundlers, async (bundler, t) => { const { files, tmpDir } = await zipFixture(t, 'node-fetch', { opts: { jsBundler: bundler } }) await unzipFiles(files) From cd81510f983f3edf4f48f140133ba827fa55f83c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eduardo=20Bou=C3=A7as?= Date: Tue, 2 Mar 2021 13:05:33 +0000 Subject: [PATCH 4/7] feat: add plugin for native bindings --- src/node_bundler/index.js | 8 ++- src/node_bundler/plugins.js | 60 +++++-------------- src/runtimes/node.js | 26 +++++--- .../node_modules/node-fetch/lib/index.js | 0 .../node_modules/node-fetch/lib/index.mjs | 0 .../node_modules/node-fetch/package.json | 0 tests/main.js | 14 ++++- 7 files changed, 49 insertions(+), 59 deletions(-) rename tests/fixtures/node-fetch/{.netlify/plugins => }/node_modules/node-fetch/lib/index.js (100%) rename tests/fixtures/node-fetch/{.netlify/plugins => }/node_modules/node-fetch/lib/index.mjs (100%) rename tests/fixtures/node-fetch/{.netlify/plugins => }/node_modules/node-fetch/package.json (100%) diff --git a/src/node_bundler/index.js b/src/node_bundler/index.js index ba59b200f..ba63bd59d 100644 --- a/src/node_bundler/index.js +++ b/src/node_bundler/index.js @@ -12,6 +12,7 @@ const pUnlink = promisify(fs.unlink) const bundleJsFile = async function ({ additionalModulePaths, + basePath, destFilename, destFolder, externalModules = [], @@ -22,13 +23,14 @@ const bundleJsFile = async function ({ const external = [...new Set([...externalModules, ...ignoredModules])] const jsFilename = `${basename(destFilename, extname(destFilename))}.js` const bundlePath = join(destFolder, jsFilename) + const pluginContext = {} // esbuild's async build API throws on Node 8.x, so we switch to the sync // version for that version range. const supportsAsyncAPI = semver.satisfies(process.version, '>=9.x') // The sync API does not support plugins. - const plugins = supportsAsyncAPI ? getPlugins({ additionalModulePaths }) : undefined + const plugins = supportsAsyncAPI ? getPlugins({ additionalModulePaths, basePath, context: pluginContext }) : undefined // eslint-disable-next-line node/no-sync const buildFunction = supportsAsyncAPI ? esbuild.build : esbuild.buildSync @@ -41,6 +43,7 @@ const bundleJsFile = async function ({ nodePaths: additionalModulePaths, platform: 'node', plugins, + resolveExtensions: ['.js', '.jsx', '.mjs', '.cjs', '.json'], target: ['es2017'], }) const cleanTempFiles = async () => { @@ -50,8 +53,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 index 96ec92859..4e30d1c21 100644 --- a/src/node_bundler/plugins.js +++ b/src/node_bundler/plugins.js @@ -1,58 +1,26 @@ -// This plugin addresses an edge case with `node-fetch`. The module's `main` -// export points to an ambiguous path (`lib/index`) which, by default, is -// resolved to an ESM file by esbuild. This plugin hijacks `require()` calls -// to the module and forces the path to be `lib/index.js`. -const getNodeFetchHandlerPlugin = ({ additionalModulePaths }) => ({ - name: 'node-fetch-handler', - setup(build) { - build.onResolve({ filter: /^node-fetch$/ }, ({ kind, path }) => { - if (kind !== 'require-call') { - return - } - - if (additionalModulePaths.length === 0) { - return { - path: require.resolve(path), - } - } +const { relative, resolve } = require('path') - // If we have additional module paths, these need to be included in the - // `require.resolve` call. Unfortunately, the `options.path` parameter - // doesn't allow us to simply add a path to the ones it uses by default. - // We either use the defaults or we have to specify all the paths. This - // can be done by merging `require.resolve.paths` with the paths we want - // to add, but that primitive is not available in Node 8. In that case, - // we re-try the `require.resolve` call with the additional paths if it - // fails initially. - - // It's safe to disable this rule because we're checking whether the - // function exists before attempting to use it. - // - // @todo Remove once we drop support for Node 8. - // eslint-disable-next-line node/no-unsupported-features/node-builtins - const getDefaultRequirePaths = require.resolve.paths +const getNodeBindingHandlerPlugin = ({ basePath, context }) => ({ + name: 'node-binding-handler', + setup(build) { + // We purposely want to mutate a context that is shared between plugins. + // eslint-disable-next-line fp/no-mutation, no-param-reassign + context.nodeBindings = new Set() - if (typeof getDefaultRequirePaths !== 'function') { - try { - return { - path: require.resolve(path), - } - } catch (_) { - return { - path: require.resolve(path, { paths: additionalModulePaths }), - } - } - } + build.onResolve({ filter: /\.node$/ }, (args) => { + const fullPath = resolve(args.resolveDir, args.path) + const resolvedPath = relative(basePath, fullPath) - const paths = [...getDefaultRequirePaths(path), ...additionalModulePaths] + context.nodeBindings.add(fullPath) return { - path: require.resolve(path, { paths }), + external: true, + path: `./${resolvedPath}`, } }) }, }) -const getPlugins = ({ additionalModulePaths }) => [getNodeFetchHandlerPlugin({ additionalModulePaths })] +const getPlugins = ({ basePath, context }) => [getNodeBindingHandlerPlugin({ basePath, context })] module.exports = { getPlugins } diff --git a/src/runtimes/node.js b/src/runtimes/node.js index c7121f9f3..9079cfece 100644 --- a/src/runtimes/node.js +++ b/src/runtimes/node.js @@ -51,6 +51,7 @@ const getSrcFilesAndExternalModules = async function ({ } } +// eslint-disable-next-line max-statements const zipFunction = async function ({ destFolder, extension, @@ -80,9 +81,10 @@ const zipFunction = async function ({ jsBundler, 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, @@ -96,30 +98,38 @@ 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, ignoredModules: [...ignoredModulesFromConfig, ...ignoredModulesFromSpecialCases], + srcDir, srcFile: mainFile, }) const bundlerWarnings = data.warnings.length === 0 ? undefined : data.warnings + // We're adding the bundled file to the zip, but we want it to have the same + // name and path as the original, unbundled file. For this, we use an alias.. + const aliases = { + [mainFile]: bundlePath, + } + const srcFilesAfterBundling = [...srcFiles, ...data.additionalSrcFiles] + const dirnames = srcFilesAfterBundling.map((filePath) => normalize(dirname(filePath))) + const basePath = commonPathPrefix([...dirnames, mainFilePath]) + try { await zipNodeJs({ - // We're adding the bundled file to the zip, but we want it to have the same - // name and path as the original, unbundled file. For this, we use an alias.. - aliases: { - [mainFile]: bundlePath, - }, - basePath: commonPathPrefix([...dirnames, dirname(mainFile)]), + aliases, + basePath, destFolder, destPath, filename, mainFile, pluginsModulesPath, - srcFiles, + srcFiles: srcFilesAfterBundling, }) } finally { await cleanTempFiles() diff --git a/tests/fixtures/node-fetch/.netlify/plugins/node_modules/node-fetch/lib/index.js b/tests/fixtures/node-fetch/node_modules/node-fetch/lib/index.js similarity index 100% rename from tests/fixtures/node-fetch/.netlify/plugins/node_modules/node-fetch/lib/index.js rename to tests/fixtures/node-fetch/node_modules/node-fetch/lib/index.js diff --git a/tests/fixtures/node-fetch/.netlify/plugins/node_modules/node-fetch/lib/index.mjs b/tests/fixtures/node-fetch/node_modules/node-fetch/lib/index.mjs similarity index 100% rename from tests/fixtures/node-fetch/.netlify/plugins/node_modules/node-fetch/lib/index.mjs rename to tests/fixtures/node-fetch/node_modules/node-fetch/lib/index.mjs diff --git a/tests/fixtures/node-fetch/.netlify/plugins/node_modules/node-fetch/package.json b/tests/fixtures/node-fetch/node_modules/node-fetch/package.json similarity index 100% rename from tests/fixtures/node-fetch/.netlify/plugins/node_modules/node-fetch/package.json rename to tests/fixtures/node-fetch/node_modules/node-fetch/package.json diff --git a/tests/main.js b/tests/main.js index 556109aa2..b93640f57 100644 --- a/tests/main.js +++ b/tests/main.js @@ -56,11 +56,19 @@ testBundlers('Zips Node.js function files', [ESBUILD, ESBUILD_ZISI, DEFAULT], as }) 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 }, + const { files, tmpDir } = await zipNode(t, 'node-module-native', { + opts: { jsBundler: bundler }, }) + const requires = await getRequires({ filePath: resolve(tmpDir, 'src/function.js') }) + t.true(files.every(({ runtime }) => runtime === 'js')) + t.true(await pathExists(`${tmpDir}/src/node_modules/test/native.node`)) + + if (bundler === ESBUILD_ZISI || bundler === ESBUILD) { + t.true(requires.includes('./node_modules/test/native.node')) + } else { + t.true(requires.includes('test')) + } }) testBundlers('Can require node modules', [ESBUILD, ESBUILD_ZISI, DEFAULT], async (bundler, t) => { From 24d0303accadf6d19f6fa548bc516377b047e2b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eduardo=20Bou=C3=A7as?= Date: Tue, 2 Mar 2021 13:57:34 +0000 Subject: [PATCH 5/7] chore: fix ESLint warning --- tests/fixtures/node-fetch/function.js | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/fixtures/node-fetch/function.js b/tests/fixtures/node-fetch/function.js index e72ccfdc1..f23840fba 100644 --- a/tests/fixtures/node-fetch/function.js +++ b/tests/fixtures/node-fetch/function.js @@ -1,4 +1,3 @@ -// eslint-disable-next-line import/no-unresolved const fetch = require('node-fetch') module.exports = fetch From 7b8343a25103faf041e04d430a055f8a9f316aad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eduardo=20Bou=C3=A7as?= Date: Tue, 2 Mar 2021 14:10:26 +0000 Subject: [PATCH 6/7] chore: pass populated context to plugins --- src/node_bundler/index.js | 4 +++- src/node_bundler/plugins.js | 4 ---- tests/main.js | 2 ++ 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/node_bundler/index.js b/src/node_bundler/index.js index ba63bd59d..dbff1b2c8 100644 --- a/src/node_bundler/index.js +++ b/src/node_bundler/index.js @@ -23,7 +23,9 @@ const bundleJsFile = async function ({ const external = [...new Set([...externalModules, ...ignoredModules])] const jsFilename = `${basename(destFilename, extname(destFilename))}.js` const bundlePath = join(destFolder, jsFilename) - const pluginContext = {} + 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. diff --git a/src/node_bundler/plugins.js b/src/node_bundler/plugins.js index 4e30d1c21..581515f7a 100644 --- a/src/node_bundler/plugins.js +++ b/src/node_bundler/plugins.js @@ -3,10 +3,6 @@ const { relative, resolve } = require('path') const getNodeBindingHandlerPlugin = ({ basePath, context }) => ({ name: 'node-binding-handler', setup(build) { - // We purposely want to mutate a context that is shared between plugins. - // eslint-disable-next-line fp/no-mutation, no-param-reassign - context.nodeBindings = new Set() - build.onResolve({ filter: /\.node$/ }, (args) => { const fullPath = resolve(args.resolveDir, args.path) const resolvedPath = relative(basePath, fullPath) diff --git a/tests/main.js b/tests/main.js index b93640f57..5a1cad6e3 100644 --- a/tests/main.js +++ b/tests/main.js @@ -61,6 +61,8 @@ testBundlers('Handles Node module with native bindings', [ESBUILD, ESBUILD_ZISI, }) const requires = await getRequires({ filePath: resolve(tmpDir, 'src/function.js') }) + console.log(requires) + t.true(files.every(({ runtime }) => runtime === 'js')) t.true(await pathExists(`${tmpDir}/src/node_modules/test/native.node`)) From 2e3274023a4fbe6cdc8db39862de02c4807ad3fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eduardo=20Bou=C3=A7as?= Date: Tue, 2 Mar 2021 14:29:56 +0000 Subject: [PATCH 7/7] chore: fix tests --- src/node_bundler/plugins.js | 2 +- tests/main.js | 44 +++++++++++++++++++++---------------- 2 files changed, 26 insertions(+), 20 deletions(-) diff --git a/src/node_bundler/plugins.js b/src/node_bundler/plugins.js index 581515f7a..5559b631e 100644 --- a/src/node_bundler/plugins.js +++ b/src/node_bundler/plugins.js @@ -11,7 +11,7 @@ const getNodeBindingHandlerPlugin = ({ basePath, context }) => ({ return { external: true, - path: `./${resolvedPath}`, + path: resolvedPath, } }) }, diff --git a/tests/main.js b/tests/main.js index 5a1cad6e3..1c6882ca0 100644 --- a/tests/main.js +++ b/tests/main.js @@ -11,6 +11,7 @@ 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') @@ -25,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 @@ -55,23 +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 { files, tmpDir } = await zipNode(t, 'node-module-native', { - opts: { jsBundler: bundler }, - }) - const requires = await getRequires({ filePath: resolve(tmpDir, 'src/function.js') }) - - console.log(requires) +// 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`)) + t.true(files.every(({ runtime }) => runtime === 'js')) + t.true(await pathExists(`${tmpDir}/src/node_modules/test/native.node`)) - if (bundler === ESBUILD_ZISI || bundler === ESBUILD) { - t.true(requires.includes('./node_modules/test/native.node')) - } else { - t.true(requires.includes('test')) - } -}) + 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 } }) @@ -568,12 +577,9 @@ testBundlers( }, ) -// This test relies on esbuild plugins, which aren't supported in Node 8. -const nodeFetchBundlers = semver.satisfies(version, '>=9.x') ? [ESBUILD, ESBUILD_ZISI, DEFAULT] : [DEFAULT] - testBundlers( 'Exposes the main export of `node-fetch` when imported using `require()`', - nodeFetchBundlers, + [ESBUILD, ESBUILD_ZISI, DEFAULT], async (bundler, t) => { const { files, tmpDir } = await zipFixture(t, 'node-fetch', { opts: { jsBundler: bundler } }) await unzipFiles(files)