fix(nitro): stop the v3 path importing the optional h3 peer#373
Conversation
|
@jmcgoldrick is attempting to deploy a commit to the HRCD Projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Thank you for following the naming conventions! 🙏 |
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR prevents the v3 Nitro plugin from pulling the optional Changesh3 Optional Peer Dependency Isolation
🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labelsbug 🚥 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 unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR fixes an evlog/nitro/v3 bundling regression where the v3 plugin could accidentally pull in the optional h3 peer by reusing a helper from the v2 module, and adds a regression test to prevent reintroducing the issue.
Changes:
- Extracted
extendDeferredDraininto anh3-free module so v3 imports don’t traverse the v2h3dependency. - Updated the v3 Nitro plugin to import the helper from the new module.
- Added a built-output regression test ensuring the v3 plugin’s import graph never reaches
h3, plus a patch changeset.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/evlog/test/nitro-v3/h3-peer-isolation.test.ts | Adds a dist-based regression test that asserts h3 isn’t imported from the built v3 plugin graph. |
| packages/evlog/src/nitro/enrich-drain.ts | Stops exporting the helper inline and imports it from the new module. |
| packages/evlog/src/nitro/deferred-drain.ts | New h3-free module hosting extendDeferredDrain. |
| packages/evlog/src/nitro-v3/plugin.ts | Switches v3 plugin to import the helper from the new module (avoids h3). |
| .changeset/fix-v3-h3-optional-peer-leak.md | Documents the fix and bumps evlog as a patch. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
6dc352d pointed the v3 plugin at extendDeferredDrain in nitro/enrich-drain, which imports h3 (getHeaders) for v2 — leaking the optional h3 peer into the v3 bundle. Split the helper into an h3-free nitro/deferred-drain module.
jmcgoldrick
force-pushed
the
fix/v3-h3-optional-peer-leak
branch
from
ab3ad23 to
d78a3e7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@packages/evlog/test/nitro-v3/h3-peer-isolation.test.ts`:
- Line 17: The global regex relativeRe is created with the /g flag and reused
across recursive calls to walk(), causing exec()'s lastIndex to carry over and
skip matches; fix by ensuring a fresh regex or resetting state before each
use—either recreate the pattern inside the walk() function (e.g., new RegExp or
re-declare relativeRe) or set relativeRe.lastIndex = 0 immediately before the
while (relativeRe.exec(...)) loop; update the code around the walk() function
and the import-matching while loop so nested recursive calls don't inherit a
stale lastIndex.
- Line 31: The resolver call inside the import-walking logic (the
walk(resolve(dirname(file), match[1] ?? match[2])) invocation) doesn't add an
extension for relative imports, so change the resolution to detect if the
matched path has no extension (use path.extname on match[1] ?? match[2]) and, if
missing, append ".mjs" before calling walk; update the same computed resolved
path used for readFileSync to ensure files built as .mjs are found (keep using
dirname(file) and the match variable names to locate the change).
- Around line 51-63: The current test builds a forbidden array and uses
expect(src, file).not.toContain(...) which misses h3 subpath imports like from
"h3/utils" or import("h3/router"); update the test around the forbidden array
and the loop that reads chunks (symbols: forbidden, chunks, readFileSync,
expect(...).not.toContain) to either expand forbidden to include subpath
patterns or—preferably—replace the string containment check with a regex match
that rejects any import/use of h3 or h3/* (e.g. test src against a regex for
from|import followed by 'h3' optionally followed by a slash) and assert that the
regex does not match.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: ce7ff6dd-2dac-4489-a462-ad00ef4dad43
📒 Files selected for processing (5)
.changeset/fix-v3-h3-optional-peer-leak.mdpackages/evlog/src/nitro-v3/plugin.tspackages/evlog/src/nitro/deferred-drain.tspackages/evlog/src/nitro/enrich-drain.tspackages/evlog/test/nitro-v3/h3-peer-isolation.test.ts
| } | ||
| let match: RegExpExecArray | null | ||
| while ((match = relativeRe.exec(source))) { | ||
| walk(resolve(dirname(file), match[1] ?? match[2])) |
There was a problem hiding this comment.
Add .mjs extension when resolving relative imports.
Built chunks use the .mjs extension, but line 31 resolves bare paths like ./foo without appending .mjs. The subsequent readFileSync on line 24 will fail (silently caught on line 26), causing the walk to miss reachable chunks that might import h3.
🐛 Proposed fix: append `.mjs` if no extension present
let match: RegExpExecArray | null
while ((match = relativeRe.exec(source))) {
- walk(resolve(dirname(file), match[1] ?? match[2]))
+ const relPath = match[1] ?? match[2]
+ const absPath = resolve(dirname(file), relPath)
+ walk(absPath.endsWith('.mjs') ? absPath : `${absPath}.mjs`)
}
}🤖 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 `@packages/evlog/test/nitro-v3/h3-peer-isolation.test.ts` at line 31, The
resolver call inside the import-walking logic (the walk(resolve(dirname(file),
match[1] ?? match[2])) invocation) doesn't add an extension for relative
imports, so change the resolution to detect if the matched path has no extension
(use path.extname on match[1] ?? match[2]) and, if missing, append ".mjs" before
calling walk; update the same computed resolved path used for readFileSync to
ensure files built as .mjs are found (keep using dirname(file) and the match
variable names to locate the change).
|
thanks copilot and code rabbit, good finds! Resolved and test made more consistent with others while I was at it |
🔗 Linked issue
Closes #372
📚 Description
The v3 plugin imports
extendDeferredDrainfromnitro/enrich-drain, whose top-levelimport { getHeaders } from 'h3'is for the v2 runtime — so the v3 bundle referenced the optionalh3peer even though v3 never uses it (it readsevent.req.headers). Consumers without a directh3dep then failed to build (#372).Moved the h3-free
extendDeferredDraininto its ownnitro/deferred-drainmodule; both the v3 plugin and v2enrich-drainimport it from there. The v2 path keeps itsh3import; the v3 bundle no longer referencesh3.Added a regression test that walks the built v3 plugin's import graph and asserts it never reaches
h3. No peer ranges changed; patch changeset included.📝 Checklist
Summary by CodeRabbit
Bug Fixes
Tests