fix(optimizer): non object module.exports for Node builtin modules in CJS external facade

[jamesopstad]

Description

Fixes #20047

This aims to improve the interop when using the CJS external facade. If the package is a Node builtin then the default export is used rather than the named exports.

[sapphi-red]

The current behavior is aligned with requireReturnsDefault: false of the commonjs plugin. I'm a bit worried about breaking users that rely on the current behavior.

I think we can have a special handling for node builtin modules here. I guess that would probably cover most cases.
This code would work for that.

import * as m from ${JSON.stringify(nonFacadePrefix + args.path)};
if (typeof m.default === 'function') {
  module.exports = m.default;
} else {
  module.exports = { ...m };
}

I confirmed this covers Node's builtin modules with the following script.

import module from 'node:module'

const require = module.createRequire(import.meta.url)

for (const mod of module.builtinModules) {
  const esmExport = await import(mod)
  const moduleExports = require(mod)

  if (typeof esmExport.default === 'function') {
    console.assert(esmExport.default === moduleExports, mod)
  } else {
    const esmList = Object.keys(esmExport).filter((key) => key !== 'default')
    const cjsList = Object.keys(moduleExports)
    const diffOnlyEsm = esmList.filter((key) => !cjsList.includes(key))
    const diffOnlyCjs = cjsList.filter((key) => !esmList.includes(key))
    console.assert(
      diffOnlyEsm.length === 0 && diffOnlyCjs.length === 0,
      mod,
      diffOnlyEsm
        .map((key) => `+ ${key}`)
        .concat(diffOnlyCjs.map((key) => `- ${key}`)),
    )
  }
}

[jamesopstad]

I've updated the PR to use the simpler change you suggested.

c856ac7

[sapphi-red]

By the way, converting an external require into an external import is better to avoid if possible, as it can change the semantics (as you encountered in #20047). When optimizeDeps.esbuildOptions.platform is set to node, the optimizer keeps the external require by injecting import { createRequire } from 'module'; const require = createRequire(import.meta.url);.

vite/packages/vite/src/node/optimizer/esbuildDepPlugin.ts

Line 323 in d488efd

// preserve `require` for node because it's more accurate than converting it to import

Would that help in your case? If not, I’d be curious to hear why platform: 'node' doesn't work for your case.

[jamesopstad]

By the way, converting an external require into an external import is better to avoid if possible, as it can change the semantics (as you encountered in #20047). When optimizeDeps.esbuildOptions.platform is set to node, the optimizer keeps the external require by injecting import { createRequire } from 'module'; const require = createRequire(import.meta.url);.

vite/packages/vite/src/node/optimizer/esbuildDepPlugin.ts

Line 323 in d488efd

// preserve `require` for node because it's more accurate than converting it to import

Would that help in your case? If not, I’d be curious to hear why platform: 'node' doesn't work for your case.

When I've tried using optimizeDeps.esbuildOptions.platform: 'node' in @cloudflare/vite-plugin I get this error:

TypeError: (0 , vite_ssr_import_0.createRequire) is not a function

We may be doing something else wrong but I think keeping platform: 'neutral' also felt more consistent because nodejs_compat is an optional feature in the Workers runtime.

[sapphi-red]

Thanks for the change! I realize my earlier comment might have been misleading, sorry about that. What I had in mind was to apply the change only for Node builtins. So the change I was thinking of looks like this:

Suggested change

	contents: `\
	import * as m from ${JSON.stringify(nonFacadePrefix + args.path)};
	if (typeof m.default === 'function') {
	module.exports = m.default;
	} else {
	module.exports = { ...m };
	}`,
	contents: isNodeBuiltin(args.path) ? `\
	import * as m from ${JSON.stringify(nonFacadePrefix + args.path)};
	if (typeof m.default === 'function') {
	module.exports = m.default;
	} else {
	module.exports = { ...m };
	}` : `\
	import * as m from ${JSON.stringify(
	nonFacadePrefix + args.path,
	)};` + `module.exports = { ...m };`,

[jamesopstad]

Ah, OK, thanks. I was curious if we could always just use the default export for Node builtins. This modified version of your script confirms this would work.

import module from "node:module";

const require = module.createRequire(import.meta.url);

for (const mod of module.builtinModules) {
  const esmExport = await import(mod);
  const moduleExports = require(mod);

  const esmList = Object.keys(esmExport.default);
  const cjsList = Object.keys(moduleExports);
  const diffOnlyEsm = esmList.filter((key) => !cjsList.includes(key));
  const diffOnlyCjs = cjsList.filter((key) => !esmList.includes(key));

  console.assert(
    diffOnlyEsm.length === 0 && diffOnlyCjs.length === 0,
    mod,
    diffOnlyEsm
      .map((key) => `+ ${key}`)
      .concat(diffOnlyCjs.map((key) => `- ${key}`))
  );
}

Shall we go with that as it's simpler? Then the code could be:

          contents: `\
import * as m from ${JSON.stringify(nonFacadePrefix + args.path)};
module.exports = ${isNodeBuiltin(args.path) ? 'm.default' : '{ ...m }'};
`,

[sapphi-red]

Ah, that's true. I think we can go with your one.

[jamesopstad]

I've made the changes - 0c0e387. Hopefully that's enough to solve the issues here.

It looks like the getAugmentedNamespace function used in @rollup/plugin-commonjs is quite a bit cleverer in how it handles these things. Out of interest, will Rolldown in Vite means that all this behaviour becomes more aligned in dev and build?

[sapphi-red]

It seems the Windows CI is failing because of "node:dummy-builtin" in dependencies of dep-cjs-with-external-deps.
https://github.com/vitejs/vite/pull/20048/files#diff-f7e4e2e9348777fb2e4dc2fa297148f6b58221e69a79143a09025358dfb8c204R8
When I removed it, the ERR_PNPM_SYMLINK_FAILED  Maximum call stack size exceeded error didn't happen.
Would you consider moving the test to playground/ssr-webworker? That way, we can avoid having node:* in the dependencies.

Out of interest, will Rolldown in Vite means that all this behaviour becomes more aligned in dev and build?

It will generally be more aligned. However, in the short term, this specific part won't be aligned by that (full-bundled mode would help in the long term).
Regarding that matter, you might be interested in this issue: rolldown/rolldown#4575
To summarize: Vite / Rollup have this require->import conversion mechanism, while esbuild / Rolldown does not have it, as such a conversion isn't semantically correct.

[jamesopstad]

It was the colon in the package name that was causing the issue on Windows. I've imported the dummy package as stream so that it will be treated like a Node builtin and added a comment.

[sapphi-red]

When I've tried using optimizeDeps.esbuildOptions.platform: 'node' in @cloudflare/vite-plugin I get this error:

TypeError: (0 , vite_ssr_import_0.createRequire) is not a function

We may be doing something else wrong but I think keeping platform: 'neutral' also felt more consistent because nodejs_compat is an optional feature in the Workers runtime.

Ah, I think that's probably because workerd doesn't support modules.createRequire. That makes sense.

[sapphi-red]

Thanks!

[patak-cat]

/ecosystem-ci run

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/vite@20048

commit: 19f7096

[vite-ecosystem-ci]

📝 Ran ecosystem CI on 19f7096: Open

suite	result	latest scheduled
analogjs	❌ failure	❌ failure
histoire	❌ failure	✅ success
laravel	❌ failure	❌ failure
previewjs	❌ failure	✅ success
nuxt	✅ success	❌ failure
qwik	❌ failure	✅ success
vike	❌ failure	✅ success
vitest	❌ failure	❌ failure
waku	❌ failure	✅ success

✅ ladle, astro, marko, quasar, rakkas, sveltekit, storybook, vite-plugin-pwa, unocss, react-router, vite-environment-examples, vite-plugin-svelte, vite-plugin-react, vite-plugin-vue, vite-plugin-cloudflare, vitepress, vite-setup-catalogue, vuepress

[patak-cat]

See #20115 (comment) about the failures in CI.