feat: experimental `--node-compat` / `config.node_compat` #869

threepointone · 2022-04-30T17:02:22Z

This adds an experimental node.js compatibility mode. It can be enabled by adding node_compat = true in wrangler.toml, or by passing --node-compat as a command line arg for dev/publish commands. This is currently powered by @esbuild-plugins/node-globals-polyfill (which in itself is powered by rollup-plugin-node-polyfills).

We'd previously added this, and then removed it because the quality of the polyfills isn't great. We're reintroducing it regardless so we can start getting feedback on its usage, and it sets up a foundation for replacing it with our own, hopefully better maintained polyfills.

Of particular note, this means that what we promised in https://blog.cloudflare.com/announcing-stripe-support-in-workers/ now actually works.

This patch also addresses some dependency issues, specifically leftover entries in package-lock.json.

changeset-bot · 2022-04-30T17:02:24Z

🦋 Changeset detected

Latest commit: be5bd54

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
wrangler	Patch

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

github-actions · 2022-04-30T17:03:49Z

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.developers.workers.dev/runs/2259078223/npm-package-wrangler-869

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.developers.workers.dev/prs/869/npm-package-wrangler-869

Or you can use npx with this latest build directly:

npx https://prerelease-registry.developers.workers.dev/runs/2259078223/npm-package-wrangler-869 dev path/to/script.js

threepointone · 2022-05-01T08:37:51Z

🤔 In a future PR, I'm also tempted to replace node-fetch/isomorphic-fetch/undici by default with simple proxies to a regular fetch

JacobMGEvans · 2022-05-02T14:51:43Z

packages/wrangler/src/config/validation.ts

+  validateOptionalProperty(
+    diagnostics,
+    "",
+    "node_compat",
+    rawConfig.node_compat,
+    "boolean"
+  );


Indirectly related to this PR, however, I was thinking we could add a optional messaging to the validation framework.

That would eliminate the need for things like

if (nodeCompat) { logger.warn( "Enabling node.js compatibility mode for builtins and globals. This is experimental and has serious tradeoffs. Please see https://github.com/ionic-team/rollup-plugin-node-polyfills/ for more details." ); }

JacobMGEvans · 2022-05-02T14:58:33Z

🤔 In a future PR, I'm also tempted to replace node-fetch/isomorphic-fetch/undici by default with simple proxies to a regular fetch

Is this to solve the Node 18 fetch bug?

JacobMGEvans

LGTM.

threepointone · 2022-05-02T15:09:54Z

Is this to solve the Node 18 fetch bug?

no, this is because those libs just don't work on Workers, and are meant as polyfills for fetch, which we provide natively anyway.

caass

LGTM

caass · 2022-05-02T15:59:46Z

examples/node-app/src/index.js

+export default {
+  async fetch() {
+    // make sure path actually works
+    return new Response(path.join("path/to", "some-file"));


What will this actually return?

just a string, 'path/to/some-file'. This project is simply a sanity test if and when you need to test node-compat. Honestly I might just delete this folder since it's not like we run it during CI or anything.

caass · 2022-05-02T16:01:41Z

packages/wrangler/src/index.tsx

+      const nodeCompat = args.nodeCompat ?? config.node_compat;
+      if (nodeCompat) {
+        logger.warn(
+          "Enabling node.js compatibility mode for builtins and globals. This is experimental and has serious tradeoffs. Please see https://github.com/ionic-team/rollup-plugin-node-polyfills/ for more details."


Nice 👍🏻

This adds an experimental node.js compatibility mode. It can be enabled by adding `node_compat = true` in `wrangler.toml`, or by passing `--node-compat` as a command line arg for `dev`/`publish` commands. This is currently powered by `@esbuild-plugins/node-globals-polyfill` (which in itself is powered by `rollup-plugin-node-polyfills`). We'd previously added this, and then removed it because the quality of the polyfills isn't great. We're reintroducing it regardless so we can start getting feedback on its usage, and it sets up a foundation for replacing it with our own, hopefully better maintained polyfills. Of particular note, this means that what we promised in https://blog.cloudflare.com/announcing-stripe-support-in-workers/ now actually works. This patch also addresses some dependency issues, specifically leftover entries in `package-lock.json`.

isaac-mcfadyen · 2022-05-02T17:27:10Z

Hey, just took a look at this PR and had one possible concern.

I've been dealing some some people polyfilling using esbuild and the Node polyfill plugins, and they were having some trouble with the Worker's WebCrypto.

Eventually we figured out that esbuild was injecting the crypto polyfill which was overriding the Worker's WebCrypto. As the plugin says, Crypto is not shimmed and and we just provide the commonjs one from browserify and it will likely not work, if you really want it please pass {crypto: true} as an option. meaning it is indeed automatically polyfilled (with browserifys version which won't work in Workers)

I believe the way we finally resolved it was to disable the crypto polyfill from this plugin. Is that something that we need to consider here?

threepointone · 2022-05-02T17:48:51Z

Interesting, I had no idea. Could you make a repo that reproduces the problem? I'm surprised to hear that the polyfill was overriding the global Crypto, is that what you're saying? I'm absolutely open to using better polyfills, or disabling ones that don't work right now, etc.

isaac-mcfadyen · 2022-05-02T17:49:28Z

Yup, I'd make an MRE ASAP!

isaac-mcfadyen · 2022-05-02T18:07:51Z

Alright, so it looks like this issue only occurs if the user imports crypto, which they shouldn't be doing anyway because WebCrypto is just a global variable.

This makes sense but it still might be worth disabling the crypto polyfill because the user gets an error when trying to use crypto at deploy time with it disabled and it just silently breaks things when enabled.

For reference, repo is attached. npm run publish:vanilla doesn't do polyfilling and so things throw errors, and npm run publish:node does do polyfilling which breaks things but doesn't throw an error.

https://github.com/isaac-mcfadyen/polyfill-wrangler-mre

threepointone · 2022-05-02T18:30:02Z

Thanks for digging into this, I appreciate it. There's no first class way to disable crypto simply in the poyfill we've used, so doing so will be a bit of effort. But we should definitely do it, sooner that later. I filed #875 to track it. Thanks again for bringing this to our attention!

threepointone requested a review from caass April 30, 2022 17:02

threepointone requested a review from petebacondarwin as a code owner April 30, 2022 17:02

threepointone force-pushed the node-compat branch 2 times, most recently from e48ed7d to 70d29ba Compare April 30, 2022 17:10

threepointone force-pushed the node-compat branch from 70d29ba to 34b31eb Compare May 1, 2022 09:31

threepointone added this to the 2.0 milestone May 2, 2022

JacobMGEvans reviewed May 2, 2022

View reviewed changes

JacobMGEvans approved these changes May 2, 2022

View reviewed changes

caass approved these changes May 2, 2022

View reviewed changes

threepointone force-pushed the node-compat branch from 34b31eb to be5bd54 Compare May 2, 2022 16:19

threepointone merged commit f1423bf into main May 2, 2022

threepointone deleted the node-compat branch May 2, 2022 16:25

github-actions bot mentioned this pull request May 2, 2022

Version Packages #849

Merged

threepointone mentioned this pull request May 2, 2022

fix: don't polyfill crypto even when node_compat = true #875

Closed

mglikesbikes mentioned this pull request May 5, 2022

adapters shouldn't pass arbitrary options to esbuild sveltejs/kit#4639

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: experimental `--node-compat` / `config.node_compat` #869

feat: experimental `--node-compat` / `config.node_compat` #869

threepointone commented Apr 30, 2022

changeset-bot bot commented Apr 30, 2022 •

edited

Loading

github-actions bot commented Apr 30, 2022 •

edited

Loading

threepointone commented May 1, 2022

JacobMGEvans May 2, 2022 •

edited

Loading

JacobMGEvans commented May 2, 2022

JacobMGEvans left a comment

threepointone commented May 2, 2022

caass left a comment

caass May 2, 2022

threepointone May 2, 2022

caass May 2, 2022

isaac-mcfadyen commented May 2, 2022 •

edited

Loading

threepointone commented May 2, 2022

isaac-mcfadyen commented May 2, 2022

isaac-mcfadyen commented May 2, 2022 •

edited

Loading

threepointone commented May 2, 2022

feat: experimental --node-compat / config.node_compat #869

feat: experimental --node-compat / config.node_compat #869

Conversation

threepointone commented Apr 30, 2022

changeset-bot bot commented Apr 30, 2022 • edited Loading

🦋 Changeset detected

github-actions bot commented Apr 30, 2022 • edited Loading

threepointone commented May 1, 2022

JacobMGEvans May 2, 2022 • edited Loading

Choose a reason for hiding this comment

JacobMGEvans commented May 2, 2022

JacobMGEvans left a comment

Choose a reason for hiding this comment

threepointone commented May 2, 2022

caass left a comment

Choose a reason for hiding this comment

caass May 2, 2022

Choose a reason for hiding this comment

threepointone May 2, 2022

Choose a reason for hiding this comment

caass May 2, 2022

Choose a reason for hiding this comment

isaac-mcfadyen commented May 2, 2022 • edited Loading

threepointone commented May 2, 2022

isaac-mcfadyen commented May 2, 2022

isaac-mcfadyen commented May 2, 2022 • edited Loading

threepointone commented May 2, 2022

feat: experimental `--node-compat` / `config.node_compat` #869

feat: experimental `--node-compat` / `config.node_compat` #869

changeset-bot bot commented Apr 30, 2022 •

edited

Loading

github-actions bot commented Apr 30, 2022 •

edited

Loading

JacobMGEvans May 2, 2022 •

edited

Loading

isaac-mcfadyen commented May 2, 2022 •

edited

Loading

isaac-mcfadyen commented May 2, 2022 •

edited

Loading