Repository navigation
fix(build): externalize node: builtins as node-commonjs to fix CJS interop - #80
Conversation
…terop
Bundled CJS deps using TypeScript CJS interop helpers crashed at runtime with
"TypeError: Right-hand side of 'instanceof' is not callable". With ESM output,
the previous regex external resolved with the default module type, making
require("node:stream") return an ESM namespace object instead of the real
Stream constructor.
Fixes by replacing the regex with a function external that forces the
node-commonjs type for node: prefixed requests so createRequire semantics are
preserved. Adds a regression integration test. Also bumps @rsbuild/core 1
to 2 and other dependencies.
Closes #79.
Signed-off-by: C. Spencer Beggs <spencer@beggs.codes>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![savvy-web-bot[bot]](https://avatars.githubusercontent.com/in/1855974?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
Current Commit: 223d833
Summary
Clean, well-targeted bug fix. The root cause analysis in the PR description is accurate, the one-line change in build-live.ts is the minimal correct fix, and the regression test is exactly the right way to lock this in.
Validation Checks
| Check | Status |
|---|---|
| PR Title Validation | ✅ success |
| Conventional Commits | ✅ success |
| Code Quality (Biome) | ✅ success |
| Markdown | ✅ success |
| Tests | ✅ success |
What's Good
The fix itself (src/services/build-live.ts:130–141) is surgical and correct. Switching from /^node:/ regexp (which inherits the default "module" external type under ESM output) to a function external that forces "node-commonjs" is exactly what rspack needs to ensure createRequire semantics are used, so bundled CJS deps get a real CommonJS module back from require("node:stream") rather than an ESM namespace object.
The regression test is excellent. Hand-authoring the exact __importDefault(require("node:stream")) interop pattern (rather than just importing a real package) makes the fixture self-contained, isolated from external registry changes, and impossible to accidentally "fix" by switching to a different real-world dep. The two-step test (existence check → runtime execution) gives a clear failure message at each stage.
Documentation kept in sync. Both CLAUDE.md and the architecture doc are updated to reflect the fix, the new node: external type decision, and the new integration test structure. The design decision table entry at the architecture doc clearly explains the "why" behind choosing a function external over a regexp.
Changeset correctly classifies this as a patch and gives a clear description of both the bug and the fix.
One Minor Note
See inline comment on
.claude/design/github-action-builder/architecture.md:507
The pseudo-code in the architecture doc uses return { externalType: "node-commonjs" } (object form, no explicit module identifier), while the actual implementation uses \node-commonjs ${data.request}`` (string form). Both are valid rspack externals syntax, but they look like different approaches. Worth aligning the doc to show the exact form used in the implementation.
Dependency Bumps
@rsbuild/core^1.7 → ^2.0 — major version bump. The tests passing (including the new integration test which exercises the externals API directly) gives good confidence the migration is clean, but worth noting this is a significant jump in case future issues surface.yaml-effect^0.2 → ^0.6 — four minor versions in 0.x range; no issues visible from the diff.
| (data) => { | ||
| if (data.request?.startsWith("node:")) { | ||
| return { externalType: "node-commonjs" }; | ||
| } |
There was a problem hiding this comment.
The pseudo-code here shows the object form without a module identifier:
return { externalType: "node-commonjs" };But the actual implementation in src/services/build-live.ts uses the string form:
return `node-commonjs ${data.request}`;These are functionally equivalent in rspack (omitting the name in the object form falls back to using the original request string), but the doc and the code look like they're doing different things. Consider updating this pseudo-code to match the real implementation so readers aren't confused if they copy from the doc:
(data) => data.request?.startsWith("node:") ? `node-commonjs ${data.request}` : undefined,The Rsbuild Bundler Configuration snippet in the architecture doc showed the object form returning an externalType property, but the actual code in build-live.ts uses the string form. Updated the snippet to match the real implementation so readers copying from the doc get working code. Signed-off-by: C. Spencer Beggs <spencer@beggs.codes>
Summary
node:builtins with a RegExp that resolved using the default"module"external type under ESM output (output.module: true). This maderequire("node:stream")inside bundled CJS dependencies return an ESM namespace object instead of the real Stream constructor. The TypeScript CJS interop helper then produced{ default: <namespace> }, causing downstreaminstanceofchecks to throwTypeError: Right-hand side of 'instanceof' is not callable./^node:/RegExp external with a function external that forces"node-commonjs"type for allnode:prefixed requests, preserving realcreateRequiresemantics.__test__/integration/cjs-node-interop.int.test.tsreproduces the exact crash pattern (hand-authored minipass-style CJS fixture) and verifies the fix end-to-end.@rsbuild/core^1.7→^2.0,jiti,yaml-effect, and dev deps.Test plan
pnpm test— 94 unit + 2 integration tests passpnpm run typecheck— cleanpnpm run lint— cleancjs-node-interop.int.test.ts) was red before fix, green afterCloses #79.
Signed-off-by: C. Spencer Beggs spencer@beggs.codes