Broken polyfill on Cloudflare Pages after `1.17.1` upgrade

[aaronadamsCA]

What version of Remix are you using?

1.17.1

Are all your remix dependencies & dev-dependencies using the same version?

• [X] Yes

Steps to Reproduce

1. Clone repository https://github.com/aaronadamsCA/remix-issue-node-polyfills.
2. Run remix dev or deploy to Cloudflare Pages.

Expected Behavior

Works the same as v1.17.0.

Actual Behavior

When I use library sanitize-html, this results in the inclusion of polyfill "node-modules-polyfills-commonjs:tty", which crashes the Cloudflare Workers runtime.

Local dev using Wrangler v2.20.0

First a warning: ``` ▲ [WARNING] Duplicate key "constants" in object literal [duplicate-object-key] functions/[[path]].js:30122:6: 30122 │ constants, ╵ ~~~~~~~~~ The original key "constants" is here: functions/[[path]].js:30121:6: 30121 │ constants: { F_OK, R_OK, W_OK, X_OK }, ╵ ~~~~~~~~~ ``` Then an error: ``` /workspaces/remix-issue-6665/functions/[[path]].js:17285 module.exports[k4] = polyfill[k4]; ^ TypeError: Cannot assign to read only property 'ReadStream' of object '[object Object]' at node-modules-polyfills-commonjs:tty (/workspaces/remix-issue-6665/functions/[[path]].js:17285:24) at /workspaces/remix-issue-6665/functions/[[path]].js:10:46 at node_modules/picocolors/picocolors.js (/workspaces/remix-issue-6665/functions/[[path]].js:17295:15) at /workspaces/remix-issue-6665/functions/[[path]].js:10:46 at node_modules/postcss/lib/css-syntax-error.js (/workspaces/remix-issue-6665/functions/[[path]].js:17510:16) at /workspaces/remix-issue-6665/functions/[[path]].js:10:46 at node_modules/postcss/lib/postcss.js (/workspaces/remix-issue-6665/functions/[[path]].js:31823:26) at /workspaces/remix-issue-6665/functions/[[path]].js:10:46 at node_modules/sanitize-html/index.js (/workspaces/remix-issue-6665/functions/[[path]].js:31882:229) at /workspaces/remix-issue-6665/functions/[[path]].js:10:46 ```

Deploying to Cloudflare Pages results in "this deployment failed" with no details

[acidio]

FYI, I downgraded to 1.7.0 but the issue remains, maybe this is related to another dependency that was updated? 🤔

[aaronadamsCA]

My rollback to 1.17.0 fixed the issue with no other changes.

A much simpler app in the same monorepo upgraded to v1.17.1 without issue, but it didn't need to include the "node-modules-polyfills-commonjs:tty" polyfill in its build. That's where our main app is failing.

I'm looking to see if I can find any upstream issues that have been reported, nothing so far.

[acidio]

In my case it's a brand new app, I can't compare what modules were updated from 1.7.0 to 1.7.1, but I also only got this issue when I added import { json } from '@remix-run/node' to use on an action, which I believe is related to "node-modules-polyfills-commonjs:tty" as you mentioned.

[aaronadamsCA]

Our smaller app still worked post-upgrade despite including import { json } from "@remix-run/cloudflare". You may wish to try importing from "@remix-run/cloudflare" if you're developing for Cloudflare.

On my side, if nobody else chimes in with more info, I'll try to figure out the minimal reproduction.

[acidio]

You're right, I should import it from "@remix-run/cloudflare" 🤦 That works, thanks for the tip.

[silence48]

I am getting the same issue and i don't have any imports from @remix-run/node...

💿 Rebuilt in 1.2s ▲ [WARNING] Duplicate key "constants" in object literal [duplicate-object-key]

functions/[[path]].js:44234:6:
  44234 │       constants,
        ╵       ~~~~~~~~~

The original key "constants" is here:

functions/[[path]].js:44233:6:
  44233 │       constants: { F_OK, R_OK, W_OK, X_OK },
        ╵       ~~~~~~~~~

▲ [WARNING] Duplicate key "constants" in object literal [duplicate-object-key]

functions/[[path]].js:44234:6:
  44234 │       constants,
        ╵       ~~~~~~~~~

The original key "constants" is here:

    functions/[[path]].js:44233:6:
▲ [WARNING] 1 warning(s) when compiling Worker.

I am on 1.16.1 btw

[silence48]

sorry i was on 1.17.1 i had ^1.16.1 in my package.json. I pinned it to 1.16.1 and the bugs went away. I have nothing requiring the /node i always use @remix-run/cloudflare

[markdalgleish]

@aaronadamsCA I followed your reproduction steps and I'm not seeing the local dev issue you described. Could you share a minimal repro?

[aaronadamsCA]

sanitize-html appears to be the package causing the "node-modules-polyfills-commonjs:tty" polyfill to be included. I am guessing this polyfill being broken on Cloudflare Pages may wind up being an upstream issue.

I am putting together a minimal reproduction now.

[aaronadamsCA]

@markdalgleish Very minimal reproduction:

https://github.com/aaronadamsCA/remix-issue-6665

This crashes on remix dev. OP updated as well.

[markdalgleish]

I've raised a couple of PRs against esbuild-plugins-node-modules-polyfill to address this issue.

[github-actions[bot]]

🤖 Hello there,

We just published version v0.0.0-nightly-4ef7a88-20230629 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

[github-actions[bot]]

🤖 Hello there,

We just published version v0.0.0-nightly-0657c16-20230630 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

[github-actions[bot]]

🤖 Hello there,

We just published version 1.18.1-pre.2 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

[github-actions[bot]]

🤖 Hello there,

We just published version v0.0.0-nightly-7abaf9f-20230701 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

[github-actions[bot]]

🤖 Hello there,

We just published version 1.19.0-pre.0 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

[markdalgleish]

I tried this against 1.19.0-pre.0 and I was able to get it to work, but only if I made the following changes:

1) Provided a process global polyfill in server.ts:

import process from 'node:process';
globalThis.process = process;

2) I moved the import of sanitize-html into the component body as a require call rather than a top-level import:

export default function Index() {
  const sanitizeHtml = require('sanitize-html'); // Ugh

  return (
    <div
      dangerouslySetInnerHTML={{
        __html: sanitizeHtml("<p>Hello world</p>"),
      }}
    ></div>
  );
}

This seems to be because sanitize-html uses PostCSS under the hood and for some reason a file watcher is set up and the polyfill immediately calls setTimeout, but you're only allowed to do this within the scope of a request, not at the top level.

This feels like a pretty big hack to me though. You might want to look for a lighter-weight alternative to sanitize-html since it seems a bit heavy for Cloudflare.

[aaronadamsCA]

Can we maybe get this issue reopened for now until we can figure out a resolution?

You won't get any disagreement on my part that sanitize-html is quite heavy, but I'm not sure it's going to be easy to replace. Right now I'd prefer to get it working again if possible, as it did on v1.17.0 and prior. It is also a pretty good test of the polyfills themselves.

With your help, here's what I think I know so far, please correct anything that isn't right:

• In v1.17.1, Remix changed Node polyfill libraries to fix a handful of other issues.

  • The new node-modules-polyfills:tty was breaking, leading to our initial error. You fixed this upstream (thank you!).

• As a result of this change, the process global is no longer polyfilled.

  • 6696 would have added this back in v1.18.1, but this change was reverted prior to release.

• You suggested I try to polyfill process in server.ts, but this doesn't work. The process global isn't polyfilled, only globalThis.process, so I still get Uncaught ReferenceError: process is not defined.

  • Maybe I could require() the library into the front end to get around this as you suggested, but unlike the minimal repro we want to run this on the server side.

• If I patch @remix-run/dev to enable the new process polyfill myself, now I get an error in the new node-modules-polyfills:fs:

  service core:user:worker: Uncaught Error: Some functionality, such as asynchronous I/O, timeouts, and generating random values, can only be performed while handling a request.
  at et82e18l7nb.js:44634:24 in FSWatcher2._this._persist
  at et82e18l7nb.js:44649:140 in FSWatcher2.start
  at et82e18l7nb.js:44465:22 in Volume2.watch
  at et82e18l7nb.js:44716:7 in watchStdo
  at et82e18l7nb.js:46887:5 in node-modules-polyfills:fs
  at et82e18l7nb.js:79:56
  at et82e18l7nb.js:47114:5 in node-modules-polyfills-commonjs:fs
  at et82e18l7nb.js:79:56
  at et82e18l7nb.js:47122:135 in ../../node_modules/.pnpm/postcss@8.4.21/node_modules/postcss/lib/previous-map.js
  at et82e18l7nb.js:82:50

  • This is because the polyfill itself is setting up file system watchers (!), which is breaking Cloudflare Workers:

  var /* ... */ init_fs = __esm({
    "node-modules-polyfills:fs"() {
      // ...
      watchStdo("/dev/stdout", 1, console.log);
      watchStdo("/dev/stderr", 2, console.error);
      // ...
    }
  });

Let me know what you think. IMHO:

• Remix can't load node-modules-polyfills:fs if it creates file system watchers, because this breaks Cloudflare Workers. One of these things needs to change.
• There should be an easy way to re-enable the global process and/or Buffer polyfills that were removed from Remix in v1.17.1.
  • In the short run I guess a core patch would be fine; ideally this would have a dist/compiler/plugins/nodePolyfillsPlugin.js file to target for easier maintenance.
  • Ultimately I think these polyfills should be enabled as needed and/or based on configuration. Maybe some existing setting would work, like serverPlatform: "neutral".

[markdalgleish]

I dug into thefs issue and figured out why this wasn't happening with the older polyfill setup. In esbuild-plugin-polyfill-node it's actually configured to provide an empty module for fs by default, among others: https://github.com/cyco130/esbuild-plugin-polyfill-node/blob/9afcb6abaf9062a15daaffce9a14e478b365139c/src/index.ts#L143-L149. Since it's using the same JSPM polyfills, I can actually reproduce the issue with the older polyfill setup if I enable the fs polyfill.

In order to get us back to a similar setup for you, I might need to add support for empty polyfills upstream.

[aaronadamsCA]

@markdalgleish Great find, thank you! That led me to look further into the underlying polyfill; patching out these two lines in it worked around the issue, so I agree that passing an empty polyfill will "solve" the problem.

(I don't understand why an fs polyfill would watch devices on load. Maybe that's worth reporting to @jspm/core as a compatibility problem with workerd.)

IMHO, the ability for Remix apps to "officially" opt into/out of specific polyfills seems like it could be a good long-term solution for all of these ecosystem challenges. If I could set serverNodePolyfillsOptions in my config and have it passed through to the server compiler, I don't think I'd need to patch anything.

Until then, with your help I've got a two-patch workaround that lets me get back onto Remix latest, so thanks again!

@jspm__core@2.0.1.patch

```patch diff --git a/nodelibs/browser/fs.js b/nodelibs/browser/fs.js index c234dc5d07c5ac6df817477bfbaaf9f563f37375..092933de60123809a68ee641a69c1fcd2542e47d 100644 --- a/nodelibs/browser/fs.js +++ b/nodelibs/browser/fs.js @@ -4411,19 +4411,6 @@ vol.releasedFds = [2, 1, 0]; vol.openSync('/dev/stdin', 'w'); vol.openSync('/dev/stdout', 'r'); vol.openSync('/dev/stderr', 'r'); -watchStdo('/dev/stdout', 1, console.log); -watchStdo('/dev/stderr', 2, console.error); -function watchStdo(path, fd, listener) { - let oldSize = 0; - const decoder = new TextDecoder(); - vol.watch(path, 'utf8', () => { - const { size } = vol.fstatSync(fd); - const buf = Buffer.alloc(size - oldSize); - vol.readSync(fd, buf, 0, buf.length, oldSize); - oldSize = size; - listener(decoder.decode(buf, { stream: true })); - }); -} const fs = createFsFromVolume(vol); ```

@remix-run__dev@1.18.1.patch

```patch diff --git a/dist/compiler/server/compiler.js b/dist/compiler/server/compiler.js index a5143271bd5f30487d7f2b052ce9b37229add46c..880317346c3753ab85760d33d75860498dd84620 100644 --- a/dist/compiler/server/compiler.js +++ b/dist/compiler/server/compiler.js @@ -72,7 +72,11 @@ const createEsbuildConfig = (ctx, refs) => { sideEffects: false })]; if (ctx.config.serverPlatform !== "node") { - plugins.unshift(esbuildPluginsNodeModulesPolyfill.nodeModulesPolyfillPlugin()); + plugins.unshift(esbuildPluginsNodeModulesPolyfill.nodeModulesPolyfillPlugin({ + globals: { + process: true + } + })); } return { absWorkingDir: ctx.config.rootDirectory, ```

[markdalgleish]

@aaronadamsCA Relating to your issue around globals, have you tried adding this to app.root.tsx in your minimal repro?

import process from 'node:process';
globalThis.process = process;

I realise I posted earlier that I added this to server.ts but I might have got that wrong. Moving this to app/root.tsx seems to work for me. We're trying to avoid adding further API for polyfills so I'm hoping this issue with globals can be solved on your end.

[github-actions[bot]]

🤖 Hello there,

We just published version 1.19.0-pre.4 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

[aaronadamsCA]

@markdalgleish Thank you for the help and the update. I've managed to update the reproduction repository so that it works correctly without any patched packages. 🙌

Some quick observations:

• It's still unfortunate that the process global is polyfilled in root.jsx, while the process module is polyfilled in remix.config.js. I understand the desire to avoid adding API for polyfills, but I'd argue that accepting serverNodeBuiltinsPolyfill.globals would remove API for polyfills, since there would no longer be separate polyfill APIs for globals vs. modules. Hopefully you'll still consider this.

• When I set fs: "empty", this unexpectedly removed default polyfills that I had to restore myself:

  serverNodeBuiltinsPolyfill: {
  modules: {
    fs: "empty",
    path: true,
    process: true,
    tty: true,
    url: true,
  },
  },

  I expected my config would be merged with the default - pass false to disable a default polyfill, true to enable a non-default one, and so on. Just worth noting.

[github-actions[bot]]

🤖 Hello there,

We just published version 1.19.0 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

[github-actions[bot]]

🤖 Hello there,

We just published version 1.19.0 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

[meoyawn]

tried 1.19.0 with

serverNodeBuiltinsPolyfill: {
  modules: {
    fs: "empty",
    buffer: "empty",
    "fs/promises": "empty",
  },
},

but it still errors when gray-matter calls Buffer.from:

https://github.com/jonschlinkert/gray-matter/blob/ce67a86dba419381db0dd01cc84e2d30a1d1e6a5/lib/utils.js#L36

Buffer is not defined

[markdalgleish]

You'll need to add the Buffer constructor it to the global environment somewhere at the root of your app:

import { Buffer } from 'buffer';
globalThis.Buffer = Buffer;

Note that this won't work if you've set the buffer polyfill to "empty", like in your config snippet.