fix(build): experimental flag to rewrite leaked runtime require of bundled deps (#4171)#4365
Draft
pi0x wants to merge 1 commit into
Draft
fix(build): experimental flag to rewrite leaked runtime require of bundled deps (#4171)#4365pi0x wants to merge 1 commit into
pi0x wants to merge 1 commit into
Conversation
Vite's SSR build lowers a `require("react")` inside vendored CommonJS to a
runtime `__require("react")` (createRequire) because React is an SSR-external.
Once Nitro bundles React for a self-contained `.output/`, that require is left
dangling: it fails with `Cannot find module 'react'` when node_modules is not
shipped, or loads a duplicate React instance ("Invalid hook call") when it is.
Add an opt-in `experimental.cjsRequireRewrite` flag that rewrites such leaked
requires to the copy already bundled in the output. `"react"` targets the React
family; `true` covers any bundled dependency. Unresolved leaks warn instead of
shipping silently.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughA new ChangesCJS require rewrite plugin
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/unit/cjs-require.test.ts (1)
27-144: ⚡ Quick winAdd coverage for single-quoted leaked requires
Please add a case for
__require('react')(and/or another specifier) to lock in quote-agnostic rewrite behavior and avoid regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/cjs-require.test.ts` around lines 27 - 144, Add a new test case in the describe block after the first it("rewrites a leaked __require() to the bundled initializer") test to verify single-quoted leaked requires are handled correctly. Create a test similar to the existing one but use single quotes in the __require call (e.g., __require('react') instead of __require("react")) and verify that the rewrite logic still produces the correct import statement and variable assignment regardless of quote style. This ensures the rewrite behavior is quote-agnostic and prevents regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/build/plugins/cjs-require.ts`:
- Around line 28-29: The LEAK_RE regex pattern at the current location only
matches double-quoted require calls like __require("...") but fails to match
single-quoted variants like __require('...'). Update the LEAK_RE regex pattern
to match both single and double quotes around the require path argument.
Similarly, update the DEF_RE regex pattern (also mentioned in the review at line
85) to match both quote styles for the require definition pattern. Modify both
regex patterns to use a character class or alternation to capture both quote
types, ensuring that leak detection and rewriting work correctly regardless of
which quote style is used in the generated output.
---
Nitpick comments:
In `@test/unit/cjs-require.test.ts`:
- Around line 27-144: Add a new test case in the describe block after the first
it("rewrites a leaked __require() to the bundled initializer") test to verify
single-quoted leaked requires are handled correctly. Create a test similar to
the existing one but use single quotes in the __require call (e.g.,
__require('react') instead of __require("react")) and verify that the rewrite
logic still produces the correct import statement and variable assignment
regardless of quote style. This ensures the rewrite behavior is quote-agnostic
and prevents regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e93b040a-265d-4630-91e6-616d1a2cd1fc
📒 Files selected for processing (4)
src/build/plugins.tssrc/build/plugins/cjs-require.tssrc/types/config.tstest/unit/cjs-require.test.ts
Comment on lines
+28
to
+29
| const LEAK_RE = /\b__require\(\s*"([^"]+)"\s*\)/g; | ||
| const DEF_RE = /\bvar (require_[A-Za-z0-9_$]+)\s*=[^\n]*__commonJS/g; |
Contributor
There was a problem hiding this comment.
Handle single-quoted __require() calls too
LEAK_RE and the replacement regex only match __require("..."). If output uses __require('...'), the leak won’t be detected or rewritten, so runtime failures can persist in self-contained builds.
Suggested patch
-const LEAK_RE = /\b__require\(\s*"([^"]+)"\s*\)/g;
+const LEAK_RE = /\b__require\(\s*["']([^"'\\]+)["']\s*\)/g;
@@
- const requireRe = new RegExp(`__require\\(\\s*"${escapeRe(spec)}"\\s*\\)`, "g");
+ const requireRe = new RegExp(
+ `__require\\(\\s*["']${escapeRe(spec)}["']\\s*\\)`,
+ "g"
+ );Also applies to: 85-85
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/build/plugins/cjs-require.ts` around lines 28 - 29, The LEAK_RE regex
pattern at the current location only matches double-quoted require calls like
__require("...") but fails to match single-quoted variants like
__require('...'). Update the LEAK_RE regex pattern to match both single and
double quotes around the require path argument. Similarly, update the DEF_RE
regex pattern (also mentioned in the review at line 85) to match both quote
styles for the require definition pattern. Modify both regex patterns to use a
character class or alternation to capture both quote types, ensuring that leak
detection and rewriting work correctly regardless of which quote style is used
in the generated output.
commit: |
pi0
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
[ai assited] resolves #4171 via a workaround until upstream issue is fixed
Try:
"nitro": "https://pkg.pr.new/nitro@4365"inpackage.jsonnitro.config.tsaddexperimental: { cjsRequireRewrite: "react" }Vite's SSR build lowers a
require("react")inside vendored CommonJS (e.g.use-sync-external-store's shim, as shipped inside base-ui / recharts / reactflow) to a runtime__require("react")(createRequire), because React is an SSR-external. Once Nitro bundles React for a self-contained.output/, that require is left dangling:Cannot find module 'react'whennode_modulesisn't shipped (Docker runner, serverless), orInvalid hook call/ null dispatcher when it is —__requireloads a second React instance.Root cause is the Vite SSR lowering (pure Rolldown links the same nested require correctly); this is a downstream workaround.
This PR adds an opt-in
experimental.cjsRequireRewriteflag — agenerateBundlepass that rewrites leaked__require("x")to the copy ofxalready bundled in the output, so a single instance is used and the output stays self-contained."react"— targets the React family (react,react-dom,react/jsx-runtime, …).true— covers any bundled dependency that leaked a runtime require.Unresolved leaks emit a warning instead of shipping silently.