Broken polyfill on Cloudflare Pages after `1.17.1` upgrade #6665

aaronadamsCA · 2023-06-22T12:22:24Z

What version of Remix are you using?

1.17.1

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

Yes

Steps to Reproduce

Clone repository https://github.com/aaronadamsCA/remix-issue-node-polyfills.
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

The text was updated successfully, but these errors were encountered:

acidio · 2023-06-22T14:12:12Z

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

aaronadamsCA · 2023-06-22T17:01:12Z

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 · 2023-06-22T17:40:33Z

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 · 2023-06-22T17:57:17Z

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 · 2023-06-22T18:55:52Z

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

silence48 · 2023-06-23T00:15:04Z

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 · 2023-06-23T00:23:54Z

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 · 2023-06-23T00:55:59Z

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

aaronadamsCA · 2023-06-23T12:38:48Z

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 · 2023-06-23T13:14:32Z

@markdalgleish Very minimal reproduction:

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

This crashes on remix dev. OP updated as well.

markdalgleish · 2023-06-26T06:44:51Z

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

github-actions · 2023-06-29T07:08:59Z

🤖 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 · 2023-06-30T07:05:58Z

🤖 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 · 2023-06-30T18:40:21Z

🤖 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 · 2023-07-01T07:09:09Z

🤖 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 · 2023-07-13T16:29:53Z

🤖 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 · 2023-07-14T01:01:49Z

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:

Provided a process global polyfill in server.ts:

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

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 · 2023-07-14T17:06:49Z

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.
- fix(dev): polyfill Node process and Buffer globals #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/[email protected]/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 · 2023-07-17T04:02:50Z

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 · 2023-07-17T16:18:11Z

@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!

@[email protected]

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);

@[email protected]

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 · 2023-07-18T03:49:09Z

@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 · 2023-07-18T12:22:01Z

🤖 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 · 2023-07-18T17:14:39Z

@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 · 2023-07-19T18:03:01Z

🤖 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 · 2023-07-19T18:03:58Z

🤖 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 · 2023-07-19T21:46:28Z

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 · 2023-07-19T23:00:18Z

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.

aaronadamsCA added the bug:unverified label Jun 22, 2023

brophdawg11 added the adapter:cloudflare-pages label Jun 22, 2023

aaronadamsCA changed the title ~~Cloudflare Pages broken after 1.17.1 upgrade~~ Broken polyfill on Cloudflare Pages after 1.17.1 upgrade Jun 25, 2023

This was referenced Jun 26, 2023

fix: handle frozen default exports imranbarbhuiya/esbuild-plugins-node-modules-polyfill#120

Merged

feat: support process and Buffer globals imranbarbhuiya/esbuild-plugins-node-modules-polyfill#121

Merged

markdalgleish mentioned this issue Jun 27, 2023

fix(dev): polyfill Node process and Buffer globals #6696

Merged

github-actions bot closed this as completed Jun 29, 2023

aaronadamsCA mentioned this issue Jul 13, 2023

Broken node polyfill on Cloudflare Pages #6825

Closed

1 task

markdalgleish reopened this Jul 17, 2023

aaronadamsCA mentioned this issue Jul 17, 2023

🐛 BUG: JSG lock error when running locally cloudflare/workers-sdk#3289

Closed

markdalgleish mentioned this issue Jul 18, 2023

feat(dev): support empty server polyfills #6859

Merged

github-actions bot closed this as completed Jul 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Broken polyfill on Cloudflare Pages after `1.17.1` upgrade #6665

Broken polyfill on Cloudflare Pages after `1.17.1` upgrade #6665

aaronadamsCA commented Jun 22, 2023 •

edited

Loading

acidio commented Jun 22, 2023

aaronadamsCA commented Jun 22, 2023 •

edited

Loading

acidio commented Jun 22, 2023

aaronadamsCA commented Jun 22, 2023 •

edited

Loading

acidio commented Jun 22, 2023

silence48 commented Jun 23, 2023 •

edited

Loading

silence48 commented Jun 23, 2023

markdalgleish commented Jun 23, 2023

aaronadamsCA commented Jun 23, 2023

aaronadamsCA commented Jun 23, 2023 •

edited

Loading

markdalgleish commented Jun 26, 2023

github-actions bot commented Jun 29, 2023

github-actions bot commented Jun 30, 2023

github-actions bot commented Jun 30, 2023

github-actions bot commented Jul 1, 2023

github-actions bot commented Jul 13, 2023

markdalgleish commented Jul 14, 2023 •

edited

Loading

aaronadamsCA commented Jul 14, 2023 •

edited

Loading

markdalgleish commented Jul 17, 2023

aaronadamsCA commented Jul 17, 2023 •

edited

Loading

markdalgleish commented Jul 18, 2023

github-actions bot commented Jul 18, 2023

aaronadamsCA commented Jul 18, 2023 •

edited

Loading

github-actions bot commented Jul 19, 2023

github-actions bot commented Jul 19, 2023

meoyawn commented Jul 19, 2023 •

edited

Loading

markdalgleish commented Jul 19, 2023

Broken polyfill on Cloudflare Pages after 1.17.1 upgrade #6665

Broken polyfill on Cloudflare Pages after 1.17.1 upgrade #6665

Comments

aaronadamsCA commented Jun 22, 2023 • edited Loading

What version of Remix are you using?

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

Steps to Reproduce

Expected Behavior

Actual Behavior

acidio commented Jun 22, 2023

aaronadamsCA commented Jun 22, 2023 • edited Loading

acidio commented Jun 22, 2023

aaronadamsCA commented Jun 22, 2023 • edited Loading

acidio commented Jun 22, 2023

silence48 commented Jun 23, 2023 • edited Loading

silence48 commented Jun 23, 2023

markdalgleish commented Jun 23, 2023

aaronadamsCA commented Jun 23, 2023

aaronadamsCA commented Jun 23, 2023 • edited Loading

markdalgleish commented Jun 26, 2023

github-actions bot commented Jun 29, 2023

github-actions bot commented Jun 30, 2023

github-actions bot commented Jun 30, 2023

github-actions bot commented Jul 1, 2023

github-actions bot commented Jul 13, 2023

markdalgleish commented Jul 14, 2023 • edited Loading

aaronadamsCA commented Jul 14, 2023 • edited Loading

markdalgleish commented Jul 17, 2023

aaronadamsCA commented Jul 17, 2023 • edited Loading

markdalgleish commented Jul 18, 2023

github-actions bot commented Jul 18, 2023

aaronadamsCA commented Jul 18, 2023 • edited Loading

github-actions bot commented Jul 19, 2023

github-actions bot commented Jul 19, 2023

meoyawn commented Jul 19, 2023 • edited Loading

markdalgleish commented Jul 19, 2023

Broken polyfill on Cloudflare Pages after `1.17.1` upgrade #6665

Broken polyfill on Cloudflare Pages after `1.17.1` upgrade #6665

aaronadamsCA commented Jun 22, 2023 •

edited

Loading

aaronadamsCA commented Jun 22, 2023 •

edited

Loading

aaronadamsCA commented Jun 22, 2023 •

edited

Loading

silence48 commented Jun 23, 2023 •

edited

Loading

aaronadamsCA commented Jun 23, 2023 •

edited

Loading

markdalgleish commented Jul 14, 2023 •

edited

Loading

aaronadamsCA commented Jul 14, 2023 •

edited

Loading

aaronadamsCA commented Jul 17, 2023 •

edited

Loading

aaronadamsCA commented Jul 18, 2023 •

edited

Loading

meoyawn commented Jul 19, 2023 •

edited

Loading