From 424a44420b8cd5d8cc3145cb821988e75c803f5c Mon Sep 17 00:00:00 2001 From: Mark Dalgleish Date: Mon, 21 Aug 2023 07:09:25 +1000 Subject: [PATCH] fix(dev): support polyfill package imports in deps --- .../polyfill-package-imports-in-deps.md | 5 +++ packages/remix-dev/compiler/css/compiler.ts | 27 +++----------- packages/remix-dev/compiler/js/compiler.ts | 36 ++++++++++--------- packages/remix-dev/package.json | 2 +- yarn.lock | 8 ++--- 5 files changed, 34 insertions(+), 44 deletions(-) create mode 100644 .changeset/polyfill-package-imports-in-deps.md diff --git a/.changeset/polyfill-package-imports-in-deps.md b/.changeset/polyfill-package-imports-in-deps.md new file mode 100644 index 00000000000..95d6649b3cb --- /dev/null +++ b/.changeset/polyfill-package-imports-in-deps.md @@ -0,0 +1,5 @@ +--- +"@remix-run/dev": patch +--- + +Support dependencies that import polyfill packages for Node built-ins via a trailing slash (e.g. importing the `buffer` package with `var Buffer = require('buffer/').Buffer` as recommended in their readme). These imports were previously marked as external. This meant that they were left as dynamic imports in the client bundle and would throw a runtime error in the browser (e.g. `Dynamic require of "buffer/" is not supported`). diff --git a/packages/remix-dev/compiler/css/compiler.ts b/packages/remix-dev/compiler/css/compiler.ts index 6379786e7a5..0e491002b19 100644 --- a/packages/remix-dev/compiler/css/compiler.ts +++ b/packages/remix-dev/compiler/css/compiler.ts @@ -1,8 +1,6 @@ import { builtinModules as nodeBuiltins } from "node:module"; import * as esbuild from "esbuild"; -import type { RemixConfig } from "../../config"; -import { getAppDependencies } from "../../dependencies"; import { loaders } from "../utils/loaders"; import { cssTarget } from "../utils/cssTarget"; import { cssFilePlugin } from "../plugins/cssImports"; @@ -21,25 +19,6 @@ import type { Context } from "../context"; import { groupCssBundleFiles } from "./bundle"; import { writeMetafile } from "../analysis"; -const getExternals = (config: RemixConfig): string[] => { - // For the browser build, exclude node built-ins that don't have a - // browser-safe alternative installed in node_modules. Nothing should - // *actually* be external in the browser build (we want to bundle all deps) so - // this is really just making sure we don't accidentally have any dependencies - // on node built-ins in browser bundles. - let dependencies = Object.keys(getAppDependencies(config)); - let fakeBuiltins = nodeBuiltins.filter((mod) => dependencies.includes(mod)); - - if (fakeBuiltins.length > 0) { - throw new Error( - `It appears you're using a module that is built in to node, but you installed it as a dependency which could cause problems. Please remove ${fakeBuiltins.join( - ", " - )} before continuing.` - ); - } - return nodeBuiltins.filter((mod) => !dependencies.includes(mod)); -}; - const createCompilerEsbuildConfig = (ctx: Context): esbuild.BuildOptions => { return { entryPoints: { @@ -48,7 +27,11 @@ const createCompilerEsbuildConfig = (ctx: Context): esbuild.BuildOptions => { outdir: ctx.config.assetsBuildDirectory, platform: "browser", format: "esm", - external: getExternals(ctx.config), + // Node built-ins (and any polyfills) are guaranteed to never contain CSS, + // and the JS from this build will never be executed, so we can safely skip + // bundling them and leave any imports of them as-is in the generated JS. + // Any issues with Node built-ins will be caught by the browser JS build. + external: nodeBuiltins, loader: loaders, bundle: true, logLevel: "silent", diff --git a/packages/remix-dev/compiler/js/compiler.ts b/packages/remix-dev/compiler/js/compiler.ts index ab85ae9bf04..5fcb4b3c98e 100644 --- a/packages/remix-dev/compiler/js/compiler.ts +++ b/packages/remix-dev/compiler/js/compiler.ts @@ -33,23 +33,10 @@ type Compiler = { dispose: () => Promise; }; -const getExternals = (remixConfig: RemixConfig): string[] => { - // For the browser build, exclude node built-ins that don't have a - // browser-safe alternative installed in node_modules. Nothing should - // *actually* be external in the browser build (we want to bundle all deps) so - // this is really just making sure we don't accidentally have any dependencies - // on node built-ins in browser bundles. +const getFakeBuiltins = (remixConfig: RemixConfig): string[] => { let dependencies = Object.keys(getAppDependencies(remixConfig)); let fakeBuiltins = nodeBuiltins.filter((mod) => dependencies.includes(mod)); - - if (fakeBuiltins.length > 0) { - throw new Error( - `It appears you're using a module that is built in to node, but you installed it as a dependency which could cause problems. Please remove ${fakeBuiltins.join( - ", " - )} before continuing.` - ); - } - return nodeBuiltins.filter((mod) => !dependencies.includes(mod)); + return fakeBuiltins; }; const createEsbuildConfig = ( @@ -82,6 +69,15 @@ const createEsbuildConfig = ( ); } + let fakeBuiltins = getFakeBuiltins(ctx.config); + if (fakeBuiltins.length > 0) { + throw new Error( + `It appears you're using a module that is built in to Node, but you installed it as a dependency which could cause problems. Please remove ${fakeBuiltins.join( + ", " + )} before continuing.` + ); + } + let plugins: esbuild.Plugin[] = [ browserRouteModulesPlugin(ctx, /\?browser$/), cssBundlePlugin(refs), @@ -98,7 +94,14 @@ const createEsbuildConfig = ( emptyModulesPlugin(ctx, /^@remix-run\/(deno|cloudflare|node)(\/.*)?$/, { includeNodeModules: true, }), - nodeModulesPolyfillPlugin(), + nodeModulesPolyfillPlugin({ + // For the browser build, we replace any Node built-ins that don't have a + // polyfill with an empty module. This ensures the build can pass without + // having to mark all Node built-ins as external which can result in other + // issues, e.g. https://github.com/remix-run/remix/issues/5521. We then + // rely on tree-shaking to remove all unused polyfills and fallbacks. + fallback: "empty", + }), externalPlugin(/^node:.*/, { sideEffects: false }), ]; @@ -111,7 +114,6 @@ const createEsbuildConfig = ( outdir: ctx.config.assetsBuildDirectory, platform: "browser", format: "esm", - external: getExternals(ctx.config), loader: loaders, bundle: true, logLevel: "silent", diff --git a/packages/remix-dev/package.json b/packages/remix-dev/package.json index 9652a69748f..2dd28faa7b1 100644 --- a/packages/remix-dev/package.json +++ b/packages/remix-dev/package.json @@ -36,7 +36,7 @@ "chokidar": "^3.5.1", "dotenv": "^16.0.0", "esbuild": "0.17.6", - "esbuild-plugins-node-modules-polyfill": "^1.3.0", + "esbuild-plugins-node-modules-polyfill": "^1.4.0", "execa": "5.1.1", "exit-hook": "2.2.1", "express": "^4.17.1", diff --git a/yarn.lock b/yarn.lock index 12fcff44a04..a27a723ffbf 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5088,10 +5088,10 @@ es-to-primitive@^1.2.1: is-date-object "^1.0.1" is-symbol "^1.0.2" -esbuild-plugins-node-modules-polyfill@^1.3.0: - version "1.3.0" - resolved "https://registry.npmjs.org/esbuild-plugins-node-modules-polyfill/-/esbuild-plugins-node-modules-polyfill-1.3.0.tgz#aa61ca6189d54b163acc503b9fcbbbc825f28226" - integrity sha512-r/aNOvAlIaIzqJwvFHWhDGrPF/Aj5qI1zKVeHbCFpKH+bnKW1BG2LGixMd3s6hyWcZHcfdl2QZRucVuOLzFRrA== +esbuild-plugins-node-modules-polyfill@^1.4.0: + version "1.4.0" + resolved "https://registry.npmjs.org/esbuild-plugins-node-modules-polyfill/-/esbuild-plugins-node-modules-polyfill-1.4.0.tgz#2382a164fc07afc94ea4aef1d7ed46bfb4b02f1b" + integrity sha512-B+VD+hXhxbMCbIBsK9SEOrVIannCUefCNE1m4Fu0lrqK0NZKg+sMkmPZZJIg4cOXWt/RUv7AIB+VeVUlgVO8nw== dependencies: "@jspm/core" "^2.0.1" local-pkg "^0.4.3"