🐛 BUG: node_compat blocks building Workers that actually will work, prevents valid polyfills

[irvinebroque]

Which Cloudflare product(s) does this pertain to?

Wrangler core

What version(s) of the tool(s) are you using?

3.10.0

What version of Node are you using?

20.6.1

What operating system are you using?

Mac

Describe the Bug

Context

Wrangler attempts to identify uses of Node.js APIs in the Worker being built, and throws a build error if such an API is used, instructing developers to enable node_compat = true in their wrangler.toml, which polyfills/ponyfills some APIs, and suppresses this warning (enabling compatibility_flags = ["nodejs_compat"] also suppresses this warning, and makes actual implementations of Node.js APIs available)

Problem

In practice, this error is overly aggressive/greedy, and blocks people from writing Workers (or building polyfills) that use conditional imports to only attempt to import Node.js APIs if in a non-Workers environment. This causes two problems:

1. It prevents developers from authoring polyfills and other libraries that use Node.js APIs only if available, while still working on Workers without node_compat = true or compatibility_flags = ["nodejs_compat"] enabled
2. It causes friction when not actually necessary for application developers, who must enable one of these two modes to bring in a library, even if the Node.js APIs in that library won't actually be called.

Neither we nor anyone in the community can build polyfills that enable people to migrate from depending on APIs only available in Node.js, to using standards that are adopted across runtimes, unless we address this.

Conversely, if we do address this — it allows us and others to create clear paths to help people adopt new standards before they're available across all runtimes, by using cross-runtime APIs when available, and falling back to Node.js APIs when necessary.

Why have we not realized this previously

Because nearly all existing libraries, or cases where developers need to conditionally import Node.js APIs are ones where some subset of Node.js APIs are still needed to exist for the library to run (ex: @petebacondarwin's work with pg in brianc/node-postgres#2971) — in all of these cases, for unrelated reasons developers have had to enable one of the two Node.js compatibility modes anyways (typically node_compat = true). Doing so suppresses such warnings.

node_compat and nodejs_compatibility have effectively hidden this problem from view. I only wrapped my head around this more fully in attempting to write a polyfill for https://github.com/Ethan-Arrowood/socket. cc @jasnell @dom96 — I realized this almost immediately after writing this WIP code.

Show me the proof and steps to reproduce

Consider the following code:

async function foo() {
  // Only try to import node:net if not in a Workers environment
  if (navigator.userAgent !== 'Cloudflare-Workers') {
      // This should never be reached or evaluated in Workers:
      const net = await import('node:net'); // Or const net = require('node:net');
  }
  // else...
}

This works without issue when running directly in workerd: cloudflare/workerd#1250
This fails when running in Wrangler: https://github.com/irvinebroque/repro-node-aggro-mode

Esbuild sees this, and we warn you:
  ✘ [ERROR] Could not resolve "node:net"
      src/foo.ts:4:28:
       4 │         const net = require('node:net');
          ╵                             ~~~~~~~~~~
     The package "node:net" wasn't found on the file system but is built into node.
     Add "node_compat = true" to your wrangler.toml file to enable Node.js compatibility.

Why does this happen?

My understanding is that this happens because:

1. esbuild is a bundler, and we use it in Wrangler to bundle a Worker composed of many files into a single .js file
2. As such, if a conditional import exists, esbuild needs to know how to resolve it, and whether to treat as built-in external, or in the case of an internal module, bring its contents into the bundle (otherwise, no way these could be resolved when the Worker is run)
3. Put another way — while a runtime only touches an import statements that are actually reached when code executes, a bundler must traverse the whole dependency graph, even branches of that graph that are never reached. Which is what makes some tree-shaking scenarios hard to get right, the root of so many complex bundler plugins, etc.
4. When neither node_compat nor compatibility_flags = ["nodejs_compat"] are enabled, a conditional import of a Node.js API (ex: require('node:net')) is considered internal — it is an API that is not provided as a built-in. And it can't be found, and then we raise our own custom error with better guidance.
5. It is possible to effectively ignore errors where the import can't be found, by marking a set of modules or paths as external — provided by the host runtime. (what happens when one of the two modes noted above are enabled)
6. If we were to say "All Node.js APIs should always be treated as external by esbuild", we would effectively be converting a build time error into a runtime error.

Please correct ^ if inaccurate, this is from perspective of consumer of many bundlers, not having rolled my own or wrapped esbuild extensively before.

Note: Wrangler has a --no-bundle mode, added in #2769, which can traverse the module graph to understand which modules should be marked as external, resulting in a build with multiple files. (the changeset from when we added this is helpful 🙌 @penalosa ) — this is designed for custom builds and requires configuration though.

What should we do about it? (without more config and options)

Open to ideas.

One approach would be to treat this as a warning rather than an *error — in practice, converting any case where the import of a Node.js API is reached but not found from a build time error into a runtime error. We would still be giving people feedback at build time — just not blocking feedback.

We'd effectively be saying — we know that Node.js APIs are code that is often are imported only conditionally — often by libraries out of your direct codebase and control. And so we'll still warn you about potentially unresolved imports of Node.js APIs, when you don't have compatibility mode enabled. But we won't block you on it.

It's tempting, but I don't see a ton of value in implementing ^ this as a mode or option. (though that may be a helpful intermediate step to experiment and test things out) Moment it becomes a thing you have to think about, we lose people.

Am I missing something?

I keep thinking as I write up this issue that there's no way this is true, or that at minimum, we must have seen instances of this in the past. That there must be libraries out there that conditionally bring in Node.js APIs, and have some way I'm not seeing to do so without requiring developers who use Wrangler to add node_compat / nodejs_compat. What's missing from the above?

@admah for context re: Wrangler
@elithrar for context re: polyfilling connect()

Please provide a link to a minimal reproduction

https://github.com/irvinebroque/repro-node-aggro-mode

Please provide any relevant error logs

Esbuild sees this, and we warn you:
  ✘ [ERROR] Could not resolve "node:net"
      src/foo.ts:4:28:
       4 │         const net = require('node:net');
          ╵                             ~~~~~~~~~~
     The package "node:net" wasn't found on the file system but is built into node.
     Add "node_compat = true" to your wrangler.toml file to enable Node.js compatibility.

[penalosa]

This is definitely something we've seen before (see #1475), but we haven't properly addressed because of the existing node compatibility modes that disable the warning. We should definitely fix this. One solution might be:

1. Trap all imports of things that look like node internals
2. Ask esbuild to resolve it as normal, to check whether it's an installed package or intended as an external import
3. If it's an installed package, do nothing
4. If it's not an installed package, mark it as external for esbuild and:
   • If the nodejs compatibility flag is on, check whether it's in the list of supported modules else log a warning
   • If the nodejs polyfills setting in on, check whether it's in the list of supported modules else log a warning

How does that sound?

[isaac-mcfadyen]

This is definitely something we've seen before (see #1475), but we haven't properly addressed because of the existing node compatibility modes that disable the warning. We should definitely fix this. One solution might be:

1. Trap all imports of things that look like node internals

2. Ask esbuild to resolve it as normal, to check whether it's an installed package or intended as an external import

3. If it's an installed package, do nothing

4. If it's not an installed package, mark it as external for esbuild and:

   • If the nodejs compatibility flag is on, check whether it's in the list of supported modules else log a warning
   • If the nodejs polyfills setting in on, check whether it's in the list of supported modules else log a warning

How does that sound?

That sounds reasonable to me. I'd add another step to 4: if neither of the flags are on, also log a warning but allow the Worker to be published, since it's not possible to detect for sure that an import will cause an error until runtime.

[irvinebroque]

That change makes sense to me.