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

jamesopstad · 2025-05-15T18:26:47Z

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 · 2025-05-20T12:34:46Z

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

+import * as m from ${JSON.stringify(nonFacadePrefix + args.path)};
+if (typeof m.default === 'function' || (typeof m.default === 'object' && m.default !== null)) {
+  module.exports = Object.assign(m.default, m);
+} else {
+  module.exports = { ...m };
+}`,


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}`)), ) } }

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

c856ac7

sapphi-red · 2025-05-20T12:38:32Z

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 · 2025-05-22T08:00:24Z

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 · 2025-05-23T02:22:53Z

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

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


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 };`,

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 }'}; `,

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

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?

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.

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 · 2025-05-23T02:25:45Z

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-dev · 2025-05-27T07:07:05Z

/ecosystem-ci run

pkg-pr-new · 2025-05-27T07:08:29Z

Open in StackBlitz

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

commit: 19f7096

vite-ecosystem-ci · 2025-05-27T07:31:33Z

📝 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-dev

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

Support function default exports in CJS external facade

7b9372b

jamesopstad changed the title ~~Support function default exports in CJS external facade~~ fix: support function default exports in CJS external facade May 15, 2025

sapphi-red added feat: deps optimizer Esbuild Dependencies Optimization p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) labels May 20, 2025

sapphi-red reviewed May 20, 2025

View reviewed changes

Simplify changes to only change behaviour for function default exports

c856ac7

sapphi-red reviewed May 23, 2025

View reviewed changes

jamesopstad added 3 commits May 23, 2025 12:20

Use default export if module is Node builtin

0c0e387

Used stream as package name for @vitejs/test-dep-esm-dummy-node-builtin

992eb65

Merge branch 'main' into cjs-external-facade

19f7096

sapphi-red approved these changes May 27, 2025

View reviewed changes

sapphi-red changed the title ~~fix: support function default exports in CJS external facade~~ fix(optimizer): support function default exports in CJS external facade May 27, 2025

vite-ecosystem-ci bot added the trigger: preview label May 27, 2025

patak-dev approved these changes May 27, 2025

View reviewed changes

sapphi-red changed the title ~~fix(optimizer): support function default exports in CJS external facade~~ fix(optimizer): non object module.exports for Node builtin modules in CJS external facade May 27, 2025

sapphi-red merged commit 00ac6e4 into vitejs:main May 27, 2025
20 checks passed

jamesopstad mentioned this pull request May 27, 2025

[vite] Internal server error: The "superCtor.prototype" property must be of type object. Received undefined cloudflare/workers-sdk#9225

Open

Uh oh!

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

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

Uh oh!

Conversation

jamesopstad commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

sapphi-red May 20, 2025

Choose a reason for hiding this comment

Uh oh!

jamesopstad May 22, 2025

Choose a reason for hiding this comment

Uh oh!

sapphi-red commented May 20, 2025

Uh oh!

jamesopstad commented May 22, 2025

Uh oh!

sapphi-red May 23, 2025

Choose a reason for hiding this comment

Uh oh!

jamesopstad May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sapphi-red May 23, 2025

Choose a reason for hiding this comment

Uh oh!

jamesopstad May 23, 2025

Choose a reason for hiding this comment

Uh oh!

sapphi-red May 26, 2025

Choose a reason for hiding this comment

Uh oh!

jamesopstad May 26, 2025

Choose a reason for hiding this comment

Uh oh!

sapphi-red commented May 23, 2025

Uh oh!

sapphi-red left a comment

Choose a reason for hiding this comment

Uh oh!

patak-dev commented May 27, 2025

Uh oh!

pkg-pr-new bot commented May 27, 2025

Uh oh!

vite-ecosystem-ci bot commented May 27, 2025

Uh oh!

patak-dev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jamesopstad commented May 15, 2025 •

edited

Loading

jamesopstad May 23, 2025 •

edited

Loading