Skip to content

fix(node): support require in bundled vite configs#35482

Open
nathanwhitbot wants to merge 5 commits into
denoland:mainfrom
nathanwhitbot:orcha/impl-014ab6f3
Open

fix(node): support require in bundled vite configs#35482
nathanwhitbot wants to merge 5 commits into
denoland:mainfrom
nathanwhitbot:orcha/impl-014ab6f3

Conversation

@nathanwhitbot

Copy link
Copy Markdown
Contributor

Fixes #35457.

Tool-generated ESM (e.g. a Vite config timestamp module) can call a CJS-style require via the esbuild/rolldown shim. When run through an npm binary, that require was undefined. This scopes a globalThis.require = createRequire(moduleSpecifier) for npm-binary runs and eagerly bootstraps node:module so the shim resolves, plus supports require("file://...") in the CJS resolver.

The new is_npm_binary bootstrap flag is threaded through both entry paths:

  • normal run (cli/factory.rs)
  • compiled run deno compile npm:<pkg> (cli/rt/run.rs), so the shim behavior is preserved in standalone binaries.

A local npm fixture + spec (npm::vite_config_require_shim) covers it. The require shim is scoped to npm binaries, so normal ESM and browser bundles are unaffected.

@nathanwhitbot

Copy link
Copy Markdown
Contributor Author

Pushed a follow-up that removes the redundant isNpmBinary destructure in the worker bootstrap path. The main runtime still passes it to Node bootstrap; this just fixes the dlint unused binding failure.

nathanwhit and others added 3 commits June 25, 2026 00:49
Compiled npm binary runs (deno compile npm:<pkg>) must also set the new
is_npm_binary bootstrap flag so the require-shim behavior is preserved in
the standalone run path (cli/rt/run.rs), mirroring the normal-run path in
cli/factory.rs. Regenerated after the original reviewer-authored commit
cd91a5a62572d was lost with its review checkout; see
.orcha/memory/npm-binary-require-shim.md.
@nathanwhitbot

Copy link
Copy Markdown
Contributor Author

Rebased this PR onto latest main and resolved the conflict in ext/node/polyfills/01_require.js by keeping both the new lazy console stream binding from main and this PR's npm-binary globalThis.require shim. Verified with cargo test specs::npm::vite_config_require_shim.

@nathanwhit nathanwhit left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding globalThis.require is not the right fix, nor is it node compatible.

The require(file://) change is also not node compatible but more defensible because it does fix vite >=6, but it is still arguably a vite bug.

@nathanwhitbot

Copy link
Copy Markdown
Contributor Author

Pushed a follow-up that removes the npm-binary globalThis.require shim and its bootstrap plumbing. The remaining change is scoped to require("file://..."), with the fixture narrowed to exercise that resolver behavior from a CJS npm bin.

@nathanwhitbot

Copy link
Copy Markdown
Contributor Author

Pushed a fix for the debug unit-node failure: initialize() now keeps the moduleSpecifier bootstrap binding needed by worker_threads while preserving the npm binary global require-shim removal. Verified with cargo test -p unit_node_tests --test unit_node worker_threads_test -- --nocapture and the added vite_config_require_shim spec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CommonJS Vite config that require()s an ESM-entry package fails: 'Dynamic require of X.mjs is not supported'

2 participants