chore: record the pnpm dual-package require interop crash in agent feedback - #304
Open
rturnq wants to merge 2 commits into
Open
chore: record the pnpm dual-package require interop crash in agent feedback#304rturnq wants to merge 2 commits into
rturnq wants to merge 2 commits into
Conversation
|
WalkthroughThe pull request adds a bug report for an SSR bundling failure. Dual-published dependencies can resolve to ESM for CJS 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
2 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
agent-feedback/bugs.md (1)
9-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd complete reproduction metadata.
Before attributing the nondeterminism to rolldown, record the Node.js, pnpm, Vite, plugin, and rolldown versions. Include the active
resolve.conditions, both returned paths, and the fixture commit or reproduction command. This makes the upstream report reproducible.🤖 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 `@agent-feedback/bugs.md` at line 9, Update the reproduction documentation to include complete environment metadata before attributing the behavior to rolldown: record Node.js, pnpm, Vite, plugin, and rolldown versions, the active resolve.conditions, both paths returned by this.resolve, and the fixture commit or exact reproduction command.
🤖 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 `@agent-feedback/bugs.md`:
- Line 9: Update the server-build require-call resolution logic to handle root
resolution failures: when resolving the bare dependency from the root package,
catch only MODULE_NOT_FOUND and treat the root path as absent, while rethrowing
all other errors. Return the importer-resolved path when the root path is
missing or differs; preserve default resolution when both paths match.
---
Nitpick comments:
In `@agent-feedback/bugs.md`:
- Line 9: Update the reproduction documentation to include complete environment
metadata before attributing the behavior to rolldown: record Node.js, pnpm,
Vite, plugin, and rolldown versions, the active resolve.conditions, both paths
returned by this.resolve, and the fixture commit or exact reproduction command.
🪄 Autofix
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: 45da8839-e631-477e-933e-8d50a8837cb6
📒 Files selected for processing (1)
agent-feedback/bugs.md
|
|
||
| `src/index.ts` › `plugin` (SSR `resolveId` handling) | 2026-08-12 | impact:med | effort:med | ||
|
|
||
| When an SSR build bundles a dependency (e.g. a package shipping uncompiled `.marko` files, pulled in via the plugin's `noExternal` handling) whose CJS code `require()`s a dual-published package, rolldown can resolve that require to the package's ESM build; the emitted `new (init_x(), __toCommonJS(x_exports))(...)` interop then throws `TypeError: ... is not a constructor` at server startup, because the ESM build's `export default` leaves the class under `.default` of the namespace. Repro chain (from marko-js/run's `micro-frame-fetch` fixture): `@micro-frame/marko` → `make-fetch-happen@12` → `cacache@17` → `require("lru-cache")` with `lru-cache@7` dual-published — under pnpm the chain is private to `@micro-frame/marko`, so Vite cannot externalize it and bundles the whole chain; npm's flat layout externalizes and never hits it. The resolution is nondeterministic: with byte-identical resolve config, plugin pipeline, importer, and `kind: "require-call"` options, the same `this.resolve` returns `index.js` (CJS, works) in one host process and `index.mjs` (ESM, crashes) in another — a rolldown resolver defect worth reporting upstream. Adding `"require"` to `resolve.conditions` does not help: with both conditions active the package's `exports` key order (`import` first) wins. A verified fix (prototyped in marko-js/run PR #262-era branch `ryanturnquist-Mac-run-agent-triage-p2d582-pnpm-cjs-interop`): in a server-build `resolveId` hook, for a bare `require-call` specifier (`options.kind === "require-call"`) whose importer is inside `node_modules`, resolve with `createRequire(importer).resolve(specifier)` and return that absolute path **only when it differs from `createRequire(<root>/package.json).resolve(specifier)`** — the same-path case must keep the default flow so root-resolvable deps still externalize, and the difference-check (not mere root-resolvability) matters because a hoisted _different major_ at the root (lru-cache@11 vs the needed @7) otherwise defeats the guard. Skip builtins (`isBuiltin`) and relative/absolute specifiers. marko-js/run works around it today by declaring `make-fetch-happen` in its workspace-root `devDependencies` so the fixture chain externalizes. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
node <<'NODE'
const { createRequire } = require("node:module");
const rootRequire = createRequire("/tmp/coderabbit-review-root/package.json");
try {
rootRequire.resolve("__missing_dependency_for_review__");
throw new Error("Expected MODULE_NOT_FOUND");
} catch (error) {
if (error?.code !== "MODULE_NOT_FOUND") throw error;
console.log("Verified: require.resolve throws MODULE_NOT_FOUND when the package is absent");
}
NODERepository: marko-js/vite
Length of output: 228
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- target file ---'
cat -n agent-feedback/bugs.md | sed -n '1,40p'
printf '%s\n' '--- relevant package metadata ---'
rg -n --hidden --glob 'package.json' --glob 'pnpm-lock.yaml' --glob 'package-lock.json' --glob 'yarn.lock' \
'make-fetch-happen|micro-frame|lru-cache|pnpm|rolldown|vite' . | head -200
printf '%s\n' '--- related workaround references ---'
rg -n --hidden \
'createRequire|require-call|MODULE_NOT_FOUND|root.*resolve|make-fetch-happen|pnpm-cjs|noExternal' . \
-g '!node_modules' -g '!dist' -g '!build' | head -250
printf '%s\n' '--- node resolution probe ---'
node <<'NODE'
const { createRequire } = require("node:module");
const path = require("node:path");
const fs = require("node:fs");
const root = "/tmp/coderabbit-review-root";
const importer = "/tmp/coderabbit-review-importer/node_modules/example/index.js";
fs.mkdirSync(path.dirname(importer), { recursive: true });
const rootRequire = createRequire(`${root}/package.json`);
const importerRequire = createRequire(importer);
for (const [label, resolver] of [
["root", rootRequire],
["importer", importerRequire],
]) {
try {
resolver.resolve("__missing_dependency_for_review__");
console.log(`${label}: unexpected success`);
} catch (error) {
console.log(`${label}: code=${error && error.code}`);
if (!error || error.code !== "MODULE_NOT_FOUND") process.exitCode = 1;
}
}
function selectPath(rootResolver, importerResolver, specifier) {
let rootPath;
try {
rootPath = rootResolver.resolve(specifier);
} catch (error) {
if (!error || error.code !== "MODULE_NOT_FOUND") throw error;
}
const importerPath = importerResolver.resolve(specifier);
return rootPath === undefined || rootPath !== importerPath ? importerPath : undefined;
}
try {
selectPath(rootRequire, importerRequire, "__missing_dependency_for_review__");
console.log("selector: importer resolution is still required for a missing package");
} catch (error) {
console.log(`selector: code=${error && error.code}`);
}
NODERepository: marko-js/vite
Length of output: 18346
Handle missing root resolution before comparing paths.
createRequire(<root>/package.json).resolve(specifier) throws MODULE_NOT_FOUND when the dependency is unavailable from the root. Catch only MODULE_NOT_FOUND, rethrow other errors, and return the importer path when the root path is absent or differs.
🤖 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 `@agent-feedback/bugs.md` at line 9, Update the server-build require-call
resolution logic to handle root resolution failures: when resolving the bare
dependency from the root package, catch only MODULE_NOT_FOUND and treat the root
path as absent, while rethrowing all other errors. Return the importer-resolved
path when the root path is missing or differs; preserve default resolution when
both paths match.
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
1 participant
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.
Adds an
agent-feedback/bugs.mdentry for the SSR crash where a bundled dependency'srequire()of a dual-published transitive dep resolves to the ESM build under pnpm and breaks__toCommonJSinterop at startup. Includes the verified repro chain, the resolver nondeterminism findings, and a prototyped fix shape from marko-js/run triage — the issue belongs in this plugin (or upstream rolldown) rather than @marko/run.