feat(dev): make browser Node polyfills opt-in #7269
Conversation
🦋 Changeset detectedLatest commit: a837a9e The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| @@ -156,11 +149,12 @@ export const create = async ( | |||
| ): Promise<Compiler> => { | |||
| let compiler = await esbuild.context({ | |||
| ...createEsbuildConfig(ctx, refs), | |||
| write: false, | |||
There was a problem hiding this comment.
We need to manually write the result of the browser build to disk so that esbuild-plugins-node-modules-polyfill can check the build output in memory for any missing/unconfigured polyfills after tree-shaking has occured. This is important in the context of Remix since route files often depend on Node builtins regardless of whether they're used in browser code. If write is enabled, the outputFiles are not kept in memory and are not passed to the onEnd callback of the esbuild plugin.
If you're interested, here's the PR where I implemented this: imranbarbhuiya/esbuild-plugins-node-modules-polyfill#152
| @@ -100,7 +101,8 @@ export let create = async (ctx: Context): Promise<Compiler> => { | |||
| // js compilation (implicitly writes artifacts/js) | |||
| let js = await tasks.js; | |||
| if (!js.ok) throw error ?? js.error; | |||
| let { metafile, hmr } = js.value; | |||
| let { metafile, outputFiles, hmr } = js.value; | |||
| writes.js = JS.write(ctx.config, outputFiles); | |||
There was a problem hiding this comment.
As explained in my other comment, we now need to manage writing the output of the JS build to disk.
| // rely on tree-shaking to remove all unused polyfills and fallbacks. | ||
| fallback: "empty", | ||
| }), | ||
| externalPlugin(/^node:.*/, { sideEffects: false }), |
There was a problem hiding this comment.
Since esbuild-plugins-node-modules-polyfill is configured to manage fallbacks, this usage of externalPlugin was redundant.
| globals: ctx.config.browserNodeBuiltinsPolyfill?.globals ?? {}, | ||
| // Mark any unpolyfilled Node builtins in the build output as errors. | ||
| fallback: "error", | ||
| formatError({ moduleName, importer, polyfillExists }) { |
There was a problem hiding this comment.
So that we could customise errors for Remix consumers who don't have direct access to the esbuild plugin, I added support for custom error formatting here: imranbarbhuiya/esbuild-plugins-node-modules-polyfill#153.
| modules: ctx.config.browserNodeBuiltinsPolyfill?.modules ?? {}, | ||
| globals: ctx.config.browserNodeBuiltinsPolyfill?.globals ?? {}, | ||
| // Mark any unpolyfilled Node builtins in the build output as errors. | ||
| fallback: "error", |
There was a problem hiding this comment.
I added support for built-time errors for unpolyfilled/unconfigured modules here: imranbarbhuiya/esbuild-plugins-node-modules-polyfill#152
packages/remix-dev/compiler/server/plugins/serverNodeBuiltinsPolyfill.ts
Show resolved
Hide resolved
markdalgleish
force-pushed
the
markdalgleish/browser-node-polyfills
branch
from
8e3540e to
6aac9e0
Compare
|
🤖 Hello there, We just published version Thanks! |
This PR makes it so we no longer polyfill built-in Node modules by default. To allow consumers to opt back in to polyfills, this also adds a new
browserNodeBuiltinsPolyfilloption that mirrors the existingserverNodeBuiltinsPolyfill.This also adds a
globalsproperty to both of these options so Node globals likeBuffercan be enabled with a simple flag rather than needing to manually change production code to modify the global environment.A couple of the tests have been updated since they were relying on Node builtins in the browser.
I originally planned on making the
browserNodeBuiltinsPolyfilldefault to the value ofserverNodeBuiltinsPolyfillif it was provided, but in practice I found it confusing that a Remix config with justserverNodeBuiltinsPolyfilldefined would also have browser polyfills. It does mean there's a bit more boilerplate if you want to use the same polyfills across both, but I think it's better to be explicit, and it's easy to share a single config object across both since the Remix config is a JS file anyway.✨ This PR also adds a bonus feature where the dev server restarts when the Remix config changes. (Thanks @pcattori!)