Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(dev): remove boolean option for polyfills #6877

Merged
merged 3 commits into from
Jul 19, 2023
Merged

feat(dev): remove boolean option for polyfills #6877

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
16c1d76
feat(dev): remove boolean option from polyfills
markdalgleish Jul 19, 2023
54a8842
Update docs and minor code cleanup for clarity
brophdawg11 Jul 19, 2023
ae9fc81
Update older changeset
brophdawg11 Jul 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion .changeset/server-node-builtins-polyfill.md
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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