runtime: share one parsed value between import and require() of a JSON/TOML file - #35973
Open
robobun wants to merge 9 commits into
Open
runtime: share one parsed value between import and require() of a JSON/TOML file#35973robobun wants to merge 9 commits into
robobun wants to merge 9 commits into
Claude / Claude Code Review
completed
Jul 26, 2026 in 15m 16s
Code review found 2 potential issues
Found 4 candidates, confirmed 2. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | src/jsc/bindings/ModuleLoader.cpp:933-935 |
Unreachable : value fallback in reconcileDataModuleWithRequireCache |
| 🟡 Nit | test/js/bun/resolve/json-require-import-identity.test.ts:167 |
Unused fs require in require.extensions test fixture |
Annotations
Check warning on line 935 in src/jsc/bindings/ModuleLoader.cpp
claude / Claude Code Review
Unreachable `: value` fallback in reconcileDataModuleWithRequireCache
The `: value` fallback is unreachable: `exportsObject()` is `this->get(globalObject(), exportsPublicName())`, which returns `jsUndefined()` when the property is absent and only returns an empty `JSValue` on exception — but the preceding `RETURN_IF_EXCEPTION` already handled that path, so `existing` is always non-empty and `existing ?` is always true (same class of always-true `JSValue` check as the `entry ?` guard removed in 41d060c0). Either drop the ternary (`return existing;`) or, if a fallba
Check warning on line 167 in test/js/bun/resolve/json-require-import-identity.test.ts
claude / Claude Code Review
Unused `fs` require in require.extensions test fixture
Nit: `const fs = require("fs");` in this fixture is never used — the extension handler is `(m, f) => { m.exports = { overridden: true }; }` and doesn't read from disk. Leftover from the earlier draft where the handler used `fs.readFileSync`; drop the line.
Loading