🐛 BUG: Transpiler/bundler mishandling conversion of `const Foo = require('...')` in some cases

[jasnell]

Which Cloudflare product(s) does this pertain to?

Wrangler core

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

latest

What version of Node are you using?

latest

What operating system and version are you using?

Linux

Describe the Bug

See: https://github.com/cloudflare/workerd/issues/2257

Observed behavior

The Workers runtime exposes an implementation of the node:stream built-in module. The node:stream module is a CJS style module that exports the Stream class as the default export (e.g. import { default as Stream } from 'node:stream'). For a worker that is written to use require('...') syntax the transpiler/bundler in wrangler is instead exporting a copy of the module namespace rather than the default export.

Expected behavior

The source:

const Stream = require('stream');

Should be transpiled into the equivalent of:

import { default as Stream } from 'stream';

Instead of

import * as Stream from 'stream';

Steps to reproduce

See https://github.com/cloudflare/workerd/issues/2257

Chat thread for context: https://chat.google.com/room/AAAAAPVuvtA/-jd-ZmmF-eA/-jd-ZmmF-eA?cls=10

Please provide a link to a minimal reproduction

No response

Please provide any relevant error logs

No response

[IgorMinar]

@petebacondarwin could this be the culprit:

https://github.com/cloudflare/workers-sdk/blob/53acdbc00a95e621d90d225d943c36df41768571/packages/wrangler/src/deployment-bundle/esbuild-plugins/hybrid-nodejs-compat.ts#L21-L47

I never fully understood (and was lazy to learn) why we did this. From the comment in the code, it seems that we are doing this for ESM <> CJS compatibility reasons, which is unsurprisingly a problem, but I'm not sure why we need to handle it ourselves rather than let ESBuild deal with it, and also why we think that this compat transform implementation is correct and sufficient.

[petebacondarwin]

I think you are right that this is where the fix needs to go. I am not convinced that there is an esbuild solution to this. Looking around the issues in esbuild's repo it looks like there are only workarounds.

But I just had a play with the code and I think that changing line 42 above to use this block of code instead appears to work:

        import libDefault from '${path}';
        module.exports = libDefault;`,

This is arguably not valid but it appears to work in practice.

[petebacondarwin]

Fixed this in #6039 - by passing the default export through the virtual module that we create.