Skip to content

Commit

Permalink
feat(dev): remove boolean option for polyfills (#6877)
Browse files Browse the repository at this point in the history 
Co-authored-by: Matt Brophy <matt@brophy.org>
  • Loading branch information
@markdalgleish @brophdawg11
markdalgleish and brophdawg11 authored Jul 19, 2023
1 parent 6c9677c commit 84b2304
Show file tree
Hide file tree
Showing 7 changed files with 224 additions and 71 deletions.
2 changes: 1 addition & 1 deletion .changeset/server-node-builtins-polyfill.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ Add `serverNodeBuiltinsPolyfill` config option. In `remix.config.js` you can now

```js
// Disable all polyfills
exports.serverNodeBuiltinsPolyfill = false;
exports.serverNodeBuiltinsPolyfill = { modules: {} };

// Enable specific polyfills
exports.serverNodeBuiltinsPolyfill = {
Expand Down
5 changes: 5 additions & 0 deletions .changeset/wise-drinks-juggle.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@remix-run/dev": minor
---

[REMOVE] Remove boolean option from serverNodeBuiltinsPolyfill, and revert to "empty" polyfill for `fs`/`fs/promises` and `crypto` modules
63 changes: 57 additions & 6 deletions docs/file-conventions/remix-config.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -193,21 +193,72 @@ Defaults to `"cjs"`.

## serverNodeBuiltinsPolyfill

Whether to polyfill Node.js built-in modules in the server build, or a configurable subset of polyfills. Polyfills are provided by [JSPM][jspm] and configured via [esbuild-plugins-node-modules-polyfill]. Defaults to `true` for non-Node.js server platforms.
The Node.js polyfills to include in the server build when targeting non-Node.js server platforms. Polyfills are provided by [JSPM][jspm] and configured via [esbuild-plugins-node-modules-polyfill].

```js filename=remix.config.js
// Disable all polyfills
exports.serverNodeBuiltinsPolyfill = false;
exports.serverNodeBuiltinsPolyfill = {
modules: {
path: true, // Provide a JSPM polyfill
fs: "empty", // Provide an empty polyfill
},
};
```

If left unset, this config defaults to the following set of polyfills for non-Node.js server platforms:

// Enable specific polyfills
```js filename=remix.config.js
exports.serverNodeBuiltinsPolyfill = {
modules: {
crypto: true, // Provide a JSPM polyfill
fs: 'empty', // Provide an empty polyfill
_stream_duplex: true,
_stream_passthrough: true,
_stream_readable: true,
_stream_transform: true,
_stream_writable: true,
assert: true,
"assert/strict": true,
buffer: true,
console: true,
constants: true,
crypto: "empty",
diagnostics_channel: true,
domain: true,
events: true,
fs: "empty",
"fs/promises": "empty",
http: true,
https: true,
module: true,
os: true,
path: true,
"path/posix": true,
"path/win32": true,
perf_hooks: true,
process: true,
punycode: true,
querystring: true,
stream: true,
"stream/promises": true,
"stream/web": true,
string_decoder: true,
sys: true,
timers: true,
"timers/promises": true,
tty: true,
url: true,
util: true,
"util/types": true,
vm: true,
wasi: true,
worker_threads: true,
zlib: true,
},
};
```

<docs-warning>
This default behavior is changing in Remix v2 and will no longer polyfill any Node built-in modules on non-Node.js platforms by default. If your app requires polyfills, you will need to manually specify them via this setting. It's recommend to start manually specifying required polyfills for your app in v1 to ease your eventual migration to v2.
</docs-warning>

## serverPlatform

The platform the server build is targeting, which can either be `"neutral"` or
Expand Down
78 changes: 76 additions & 2 deletions docs/pages/v2.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -705,9 +705,83 @@ In your `remix.config.js`, you should specify either `serverModuleFormat: "cjs"`

## `serverNodeBuiltinsPolyfill`

We will no longer polyfill Node.js built-in modules by default for non-Node.js server platforms.
Polyfills for Node.js built-in modules will no longer be provided by default for non-Node.js server platforms.

If you are targeting a non-Node.js server platform, in your `remix.config.js` you should specify either `serverNodeBuiltinsPolyfill: true` to retain existing behavior, or `serverNodeBuiltinsPolyfill: false` to opt into the future default behavior. Alternatively, you can provide a list of built-in modules, e.g. `serverNodeBuiltinsPolyfill: ["crypto"]`.
If you are targeting a non-Node.js server platform and want to opt into the future default behavior, in `remix.config.js` you should first remove all server polyfills by providing an empty object for `serverNodeBuiltinsPolyfill.modules`:

```js filename=remix.config.js
module.exports = {
serverNodeBuiltinsPolyfill: {
modules: {},
},
};
```

You can then reintroduce any polyfills (or blank polyfills) as required.

```js filename=remix.config.js
module.exports = {
serverNodeBuiltinsPolyfill: {
modules: {
path: true,
fs: 'empty',
},
},
};
```

For reference, the complete set of default polyfills from v1 can be manually specified as follows:

```js filename=remix.config.js
module.exports = {
serverNodeBuiltinsPolyfill: {
modules: {
_stream_duplex: true,
_stream_passthrough: true,
_stream_readable: true,
_stream_transform: true,
_stream_writable: true,
assert: true,
"assert/strict": true,
buffer: true,
console: true,
constants: true,
crypto: "empty",
diagnostics_channel: true,
domain: true,
events: true,
fs: "empty",
"fs/promises": "empty",
http: true,
https: true,
module: true,
os: true,
path: true,
"path/posix": true,
"path/win32": true,
perf_hooks: true,
process: true,
punycode: true,
querystring: true,
stream: true,
"stream/promises": true,
"stream/web": true,
string_decoder: true,
sys: true,
timers: true,
"timers/promises": true,
tty: true,
url: true,
util: true,
"util/types": true,
vm: true,
wasi: true,
worker_threads: true,
zlib: true,
},
},
};
```

## Dev Server

Expand Down
2 changes: 1 addition & 1 deletion packages/remix-dev/__tests__/readConfig-test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ describe("readConfig", () => {
"serverMinify": false,
"serverMode": "production",
"serverModuleFormat": "cjs",
"serverNodeBuiltinsPolyfill": false,
"serverNodeBuiltinsPolyfill": undefined,
"serverPlatform": "node",
"tailwind": false,
"tsconfigPath": Any<String>,
Expand Down
45 changes: 5 additions & 40 deletions packages/remix-dev/compiler/server/compiler.ts
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,5 @@
import { builtinModules } from "module";
import * as esbuild from "esbuild";
import {
type NodePolyfillsOptions,
nodeModulesPolyfillPlugin,
} from "esbuild-plugins-node-modules-polyfill";
import { nodeModulesPolyfillPlugin } from "esbuild-plugins-node-modules-polyfill";

import { type Manifest } from "../../manifest";
import { loaders } from "../utils/loaders";
Expand Down Expand Up @@ -72,42 +68,11 @@ const createEsbuildConfig = (
];

if (ctx.config.serverNodeBuiltinsPolyfill) {
// These unimplemented polyfills throw an error at runtime if they're used.
// It's also possible that they'll be provided by the host environment (e.g.
// Cloudflare provides an "async_hooks" polyfill) so it's better to avoid
// them by default when server polyfills are enabled. If consumers want an
// unimplemented polyfill for some reason, they can explicitly pass a list
// of desired polyfills instead. This list was manually populated by looking
// for unimplemented browser polyfills in the jspm-core repo:
// https://github.com/jspm/jspm-core/tree/main/nodelibs/browser
let unimplemented = [
"async_hooks", // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/async_hooks.js
"child_process", // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/child_process.js
"cluster", // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/cluster.js
"dgram", // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/dgram.js
"dns", // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/dns.js
"dns/promises", // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/dns/promises.js
"http2", // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/http2.js
"net", // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/net.js
"readline", // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/readline.js
"repl", // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/repl.js
"tls", // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/tls.js
"v8", // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/v8.js
];

let defaultPolyfillOptions: NodePolyfillsOptions = {
modules: builtinModules.filter((mod) => !unimplemented.includes(mod)),
};

plugins.unshift(
nodeModulesPolyfillPlugin(
ctx.config.serverNodeBuiltinsPolyfill === true
? defaultPolyfillOptions
: {
// Ensure only "modules" option is passed to the plugin
modules: ctx.config.serverNodeBuiltinsPolyfill.modules,
}
)
nodeModulesPolyfillPlugin({
// Ensure only "modules" option is passed to the plugin
modules: ctx.config.serverNodeBuiltinsPolyfill.modules,
})
);
}

Expand Down
100 changes: 79 additions & 21 deletions packages/remix-dev/config.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import fse from "fs-extra";
import getPort from "get-port";
import NPMCliPackageJson from "@npmcli/package-json";
import { coerce } from "semver";
import type { NodePolyfillsOptions } from "esbuild-plugins-node-modules-polyfill";
import type { NodePolyfillsOptions as EsbuildPluginsNodeModulesPolyfillOptions } from "esbuild-plugins-node-modules-polyfill";

import type { RouteManifest, DefineRoutesFunction } from "./config/routes";
import { defineRoutes } from "./config/routes";
Expand Down Expand Up @@ -65,6 +65,11 @@ interface FutureConfig {
v2_routeConvention: boolean;
}

type ServerNodeBuiltinsPolyfillOptions = Pick<
EsbuildPluginsNodeModulesPolyfillOptions,
"modules"
>;

/**
* The user-provided config in `remix.config.js`.
*/
Expand Down Expand Up @@ -196,11 +201,10 @@ export interface AppConfig {
serverModuleFormat?: ServerModuleFormat;

/**
* Whether to polyfill Node.js built-in modules in the server build, or a
* configurable subset of polyfills. Defaults to `true` for non-Node.js server
* platforms.
* The Node.js polyfills to include in the server build when targeting
* non-Node.js server platforms.
*/
serverNodeBuiltinsPolyfill?: boolean | Pick<NodePolyfillsOptions, "modules">;
serverNodeBuiltinsPolyfill?: ServerNodeBuiltinsPolyfillOptions;

/**
* The platform the server build is targeting. Defaults to "node".
Expand Down Expand Up @@ -374,11 +378,10 @@ export interface RemixConfig {
serverModuleFormat: ServerModuleFormat;

/**
* Whether to polyfill Node.js built-in modules in the server build, or a
* configurable subset of polyfills. Defaults to `true` for non-Node.js server
* platforms.
* The Node.js polyfills to include in the server build when targeting
* non-Node.js server platforms.
*/
serverNodeBuiltinsPolyfill: boolean | Pick<NodePolyfillsOptions, "modules">;
serverNodeBuiltinsPolyfill?: ServerNodeBuiltinsPolyfillOptions;

/**
* The platform the server build is targeting. Defaults to "node".
Expand Down Expand Up @@ -506,18 +509,74 @@ export async function readConfig(
serverModuleFormat === "esm" ? ["module", "main"] : ["main", "module"];
serverMinify ??= false;

let serverNodeBuiltinsPolyfill: RemixConfig["serverNodeBuiltinsPolyfill"] =
serverPlatform !== "node";
let serverNodeBuiltinsPolyfill: RemixConfig["serverNodeBuiltinsPolyfill"];

if (appConfig.serverNodeBuiltinsPolyfill !== undefined) {
if (appConfig.serverNodeBuiltinsPolyfill != null) {
serverNodeBuiltinsPolyfill = appConfig.serverNodeBuiltinsPolyfill;
}

if (
serverPlatform !== "node" &&
appConfig.serverNodeBuiltinsPolyfill === undefined
) {
} else if (serverPlatform !== "node") {
serverNodeBuiltinsPolyfillWarning();
serverNodeBuiltinsPolyfill = {
modules: {
// Note: Remove this in Remix v2
// All polyfills are ultimately sourced from JSPM: https://github.com/jspm/jspm-core/tree/main/nodelibs/browser
// Polyfills we choose to disable are explicitly configured here so we can note the reason for disabling them.
// Links are provided here to make it easier to review the source code for each polyfill.
_stream_duplex: true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/_stream_duplex.js
_stream_passthrough: true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/_stream_passthrough.js
_stream_readable: true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/_stream_readable.js
_stream_transform: true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/_stream_transform.js
_stream_writable: true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/_stream_writable.js
assert: true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/assert.js
"assert/strict": true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/assert/strict.js
async_hooks: false, // Unimplemented, throws on usage: https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/async_hooks.js. Also Cloudflare Workers provides an implementation: https://developers.cloudflare.com/workers/runtime-apis/nodejs/asynclocalstorage/
buffer: true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/buffer.js
child_process: false, // Unimplemented, throws on usage: https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/child_process.js
cluster: false, // Unimplemented, throws on usage: https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/cluster.js
console: true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/console.js
constants: true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/constants.js
crypto: "empty", // Polyfill exists (https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/crypto.js) but source code is over 2MB! Also, it was "empty" in esbuild-plugin-polyfill-node which we used previously as of Remix v1.17.0: https://github.com/cyco130/esbuild-plugin-polyfill-node/blob/9afcb6abaf9062a15daaffce9a14e478b365139c/src/index.ts#L144
dgram: false, // Unimplemented, throws on usage: https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/dgram.js
diagnostics_channel: true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/diagnostics_channel.js
dns: false, // Unimplemented, throws on usage: https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/dns.js
"dns/promises": false, // Unimplemented, throws on usage: https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/dns/promises.js
domain: true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/domain.js
events: true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/events.js
fs: "empty", // Polyfill was "empty" in esbuild-plugin-polyfill-node which we used previously as of Remix v1.17.0 (https://github.com/cyco130/esbuild-plugin-polyfill-node/blob/9afcb6abaf9062a15daaffce9a14e478b365139c/src/index.ts#L143C6-L143C6). Also, the polyfill immediately throws when importing in Cloudflare Workers due to top-level setTimeout usage which is not allowed outside of the request lifecycle: https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/fs.js
"fs/promises": "empty", // See above
http: true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/http.js
http2: false, // Unimplemented, throws on usage: https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/http2.js
https: true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/https.js
module: true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/module.js
net: false, // Unimplemented, throws on usage: https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/net.js
os: true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/os.js
path: true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/path.js
"path/posix": true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/path/posix.js
"path/win32": true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/path/win32.js
perf_hooks: true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/perf_hooks.js
process: true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/process.js
punycode: true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/punycode.js
querystring: true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/querystring.js
readline: false, // Unimplemented, throws on usage: https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/readline.js
repl: false, // Unimplemented, throws on usage: https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/repl.js
stream: true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/stream.js
"stream/promises": true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/stream/promises.js
"stream/web": true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/stream/web.js
string_decoder: true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/string_decoder.js
sys: true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/sys.js
timers: true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/timers.js
"timers/promises": true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/timers/promises.js
tls: false, // Unimplemented, throws on usage: https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/tls.js
tty: true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/tty.js - Effectively not implemented, but provides `isatty` as `false` so consumers can check to avoid it
url: true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/url.js
util: true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/util.js
"util/types": true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/util/types.js
v8: false, // Unimplemented, throws on usage: https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/v8.js
vm: true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/vm.js
wasi: true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/wasi.js
worker_threads: true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/worker_threads.js
zlib: true, // https://github.com/jspm/jspm-core/blob/main/nodelibs/browser/zlib.js
},
};
}

if (appConfig.future) {
Expand Down Expand Up @@ -1014,9 +1073,8 @@ let serverNodeBuiltinsPolyfillWarning = () =>
"The `serverNodeBuiltinsPolyfill` config default option will be changing in v2",
{
details: [
"The default value will change from `true` to `false` regardless of platform.",
"You can prepare for this change by explicitly specifying `serverNodeBuiltinsPolyfill: false` or",
"`serverNodeBuiltinsPolyfill: true` if you are currently relying on them.",
"Server polyfills will no longer be provided by default for non-Node.js platforms.",
"You can prepare for this change by specifying server polyfills, or opting out entirely.",
"-> https://remix.run/docs/en/v1.19.0/pages/v2#servernodebuiltinspolyfill",
],
key: "serverNodeBuiltinsPolyfillWarning",
Expand Down

0 comments on commit 84b2304

Please sign in to comment.