Repository navigation
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughResolve package-style tsconfig "extends" by probing parent Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Updated 12:58 AM PT - Apr 18th, 2026
❌ @robobun, your commit 2177316 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 27248That installs a local version of the PR into your bun-27248 --bun |
|
✅ d9e9e — Looks good! Reviewed 2 files across |
|
Hey! Resolving this would be quite helpful for our team, any plans to merge it? |
|
@robobun investigate #6326 (comment) |
|
✅ Reproduced & fixed. The Two things were wrong:
Rebased onto main and extended the resolver to probe Pushed → #27248 |
d9e9e36 to
e9df616
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/resolver/resolver.zig`:
- Around line 4289-4303: The package-extends resolution is using the original
info for every hop, causing package extends to be resolved from the wrong root;
update the call to resolvePackagePathForTSConfigExtends so it uses a DirInfo
derived from the current config's directory (the same directory used for
relative extends via Dirname.dirname(current.abs_path)), i.e. compute a DirInfo
for current.abs_path and pass that instead of the original info when calling
resolvePackagePathForTSConfigExtends(info, current.extends) inside the while
loop so each extends hop resolves from the directory of the config that declared
it.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: beebca82-6280-4f0c-872f-dcaca18231ef
📥 Commits
Reviewing files that changed from the base of the PR and between bc7da9e and e9df61694ac9d4701c025bc3a68f0f185cd546f0.
📒 Files selected for processing (2)
src/resolver/resolver.zigtest/regression/issue/06326.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
✅ No merge conflicts detected when merging into Your branch is good to go! |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/resolver/resolver.zig`:
- Around line 4065-4067: tryParseTSConfigPath currently swallows all
parseTSConfig errors into null, allowing malformed/unreadable configs to be
treated like “not found”; change tryParseTSConfigPath so it only converts ENOENT
(file-not-found) into null and propagates any other errors returned by
parseTSConfig (i.e., return the error instead of null), and update
resolvePackagePathForTSConfigExtends callers to stop probing and
propagate/handle non-ENOENT errors from tryParseTSConfigPath rather than
continuing to the next candidate; use the existing symbols tryParseTSConfigPath,
parseTSConfig, and resolvePackagePathForTSConfigExtends to locate and apply this
change.
In `@test/regression/issue/06326.test.ts`:
- Around line 150-264: Add a new test in the "inherits experimentalDecorators
from extended config" suite that specifically verifies emitDecoratorMetadata is
inherited: create a test (e.g. "inherits emitDecoratorMetadata from extended
config") that uses the same tempDir setup as the other cases, ensures
reflect-metadata is imported (either in the test entry file or the existing
decoratorFixture), defines a class (e.g. Entity with property name) and asserts
Reflect.getMetadata("design:type", Entity.prototype, "name") is the expected
type (e.g. String) after running run(..., "index.ts"); reference symbols:
decoratorFixture, run, and Reflect.getMetadata to locate where to add the
assertion and import.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c302be4f-7495-4b15-bf63-0356e192b76f
📥 Commits
Reviewing files that changed from the base of the PR and between e9df61694ac9d4701c025bc3a68f0f185cd546f0 and b84f72db4caa3e2aa7332821c17de775df95886d.
📒 Files selected for processing (5)
src/bun.js/api/JSTranspiler.zigsrc/resolver/resolver.zigsrc/resolver/tsconfig_json.zigsrc/transpiler.zigtest/regression/issue/06326.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/resolver/resolver.zig`:
- Around line 3985-4062: The resolver skips checking a package's package.json
"tsconfig" field for bare package extends; in
resolvePackagePathForTSConfigExtends add a probe in the bare-package branch
(where has_subpath is false) that reads "<node_modules>/<extends>/package.json",
parses the JSON, and if a "tsconfig" string exists build its resolved path (join
dir.abs_path + "node_modules" + extends + "/" + tsconfigValue, normalizing
relative paths and appending ".json" if needed) and call r.tryParseTSConfigPath
on that resolved path before falling back to the existing
"<node_modules>/<extends>/tsconfig.json" probe; use existing buf, base_len,
DirInfo.abs_path, tryParseTSConfigPath, has_json_ext, and ensure to add a
regression test exercising an extends that uses package.json "tsconfig".
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 27c4a277-44eb-42b5-9644-782c3e37586e
📥 Commits
Reviewing files that changed from the base of the PR and between b84f72db4caa3e2aa7332821c17de775df95886d and e8f2548b0b2ba086aae3b8095163d35202aee25f.
📒 Files selected for processing (3)
src/resolver/resolver.zigsrc/resolver/tsconfig_json.zigtest/regression/issue/06326.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/resolver/resolver.zig (2)
3985-4062:⚠️ Potential issue | 🟠 MajorProbe
package.json#tsconfigfor bare packageextends.Bare package specifiers still only fall back to
<pkg>/tsconfig.json. Packages that publish their base config through a"tsconfig"field inpackage.jsonwill keep failing here even though TypeScript resolves them.Does TypeScript resolve `tsconfig.json` `"extends": "@scope/pkg"` by reading the package's `package.json` `"tsconfig"` field before falling back to `<pkg>/tsconfig.json`?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/resolver/resolver.zig` around lines 3985 - 4062, The resolver currently skips checking package.json for a "tsconfig" export when extends is a bare package; update resolvePackagePathForTSConfigExtends to, for the bare-package branch (when has_subpath is false), after constructing node_modules/<extends> and before/alongside trying "<pkg>/tsconfig.json", load and parse node_modules/<pkg>/package.json and, if it contains a "tsconfig" string, resolve that value relative to the package root (using the same buf/base_len approach and r.fs.absBuf buffer) and pass the resolved path to r.tryParseTSConfigPath; ensure you try the literal value, the value+".json" if extension missing, and value+"/tsconfig.json" if it points at a directory, and keep using DirInfo.hasNodeModules and tryParseTSConfigPath to validate results.
4065-4080:⚠️ Potential issue | 🟠 MajorDon't treat non-ENOENT tsconfig failures as "not found".
tryParseTSConfigPath()logs EACCES/EIO/parse failures but still returnsnull, so a broken nearer config can be skipped and replaced by a farther ancestor match. That changes an actual config error into different resolution behavior.Suggested direction
-fn tryParseTSConfigPath(r: *ThisResolver, candidate: string) ?*TSConfigJSON { +fn tryParseTSConfigPath(r: *ThisResolver, candidate: string) !?*TSConfigJSON { const persistent_path = r.fs.dirname_store.append(string, candidate) catch return null; - return r.parseTSConfig(persistent_path, bun.invalid_fd) catch |err| { - switch (err) { - // Expected while probing: keep walking quietly. - error.ENOENT, error.FileNotFound, error.ENOTDIR, error.NotDir, error.IsDir, error.EISDIR => {}, - // Unexpected (EACCES, EIO, etc.): surface in debug logs so a - // file that exists but can't be read doesn't vanish silently. - else => r.log.addDebugFmt(null, logger.Loc.Empty, r.allocator, "{s} loading tsconfig.json extends {f}", .{ - `@errorName`(err), - bun.fmt.QuotedFormatter{ .text = persistent_path }, - }) catch {}, - } - return null; - }; + return r.parseTSConfig(persistent_path, bun.invalid_fd) catch |err| switch (err) { + error.ENOENT, error.FileNotFound, error.ENOTDIR, error.NotDir, error.IsDir, error.EISDIR => null, + else => err, + }; }The callers in
resolvePackagePathForTSConfigExtends()should then stop probing oncatchinstead of continuing to the next ancestor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/resolver/resolver.zig` around lines 4065 - 4080, tryParseTSConfigPath currently swallows unexpected errors (EACCES/EIO/parse failures) and returns null, letting callers continue probing ancestors; change it to propagate non-ENOENT/non-NotFound/non-IsDir errors instead of returning null so callers can stop probing. Concretely: in fn tryParseTSConfigPath (the catch after r.parseTSConfig), only swallow the expected errors (error.ENOENT, error.FileNotFound, error.ENOTDIR, error.NotDir, error.IsDir, error.EISDIR) and return null for those; for any other err, return that error (or change the function signature to return an error union) so callers like resolvePackagePathForTSConfigExtends can catch and stop probing on error rather than skipping to the next ancestor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/resolver/resolver.zig`:
- Around line 3985-4062: The resolver currently skips checking package.json for
a "tsconfig" export when extends is a bare package; update
resolvePackagePathForTSConfigExtends to, for the bare-package branch (when
has_subpath is false), after constructing node_modules/<extends> and
before/alongside trying "<pkg>/tsconfig.json", load and parse
node_modules/<pkg>/package.json and, if it contains a "tsconfig" string, resolve
that value relative to the package root (using the same buf/base_len approach
and r.fs.absBuf buffer) and pass the resolved path to r.tryParseTSConfigPath;
ensure you try the literal value, the value+".json" if extension missing, and
value+"/tsconfig.json" if it points at a directory, and keep using
DirInfo.hasNodeModules and tryParseTSConfigPath to validate results.
- Around line 4065-4080: tryParseTSConfigPath currently swallows unexpected
errors (EACCES/EIO/parse failures) and returns null, letting callers continue
probing ancestors; change it to propagate non-ENOENT/non-NotFound/non-IsDir
errors instead of returning null so callers can stop probing. Concretely: in fn
tryParseTSConfigPath (the catch after r.parseTSConfig), only swallow the
expected errors (error.ENOENT, error.FileNotFound, error.ENOTDIR, error.NotDir,
error.IsDir, error.EISDIR) and return null for those; for any other err, return
that error (or change the function signature to return an error union) so
callers like resolvePackagePathForTSConfigExtends can catch and stop probing on
error rather than skipping to the next ancestor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 88d9429e-b1e5-4469-b367-3304ee73db9e
📥 Commits
Reviewing files that changed from the base of the PR and between e8f2548b0b2ba086aae3b8095163d35202aee25f and eb142540f37e192e5e6401dfb614dc91a5f0db8b.
📒 Files selected for processing (1)
src/resolver/resolver.zig
b6cc47e to
c110a8b
Compare
c110a8b to
1297396
Compare
1297396 to
2941f5e
Compare
…ode_modules When tsconfig.json uses `"extends": "@acme/configuration/tsconfig.base.json"` (a bare package specifier), Bun was naively joining the path against the tsconfig's directory instead of resolving through node_modules. This caused inherited settings like `emitDecoratorMetadata` and `experimentalDecorators` to silently fail. This change: - Adds node_modules resolution for bare package specifiers in tsconfig extends by walking up parent directories (matching TypeScript's behavior) - Merges `experimentalDecorators` in the extends chain (was previously missing) Closes #6326 Co-Authored-By: Claude <noreply@anthropic.com>
… extends When 'extends' is a bare package name (e.g. '@tsconfig/node20'), probe <pkg>/tsconfig.json. When the subpath has no .json extension (e.g. 'expo/tsconfig.base'), probe <subpath>.json. When the subpath names a directory, probe <subpath>/tsconfig.json. Matches TypeScript's lookup behavior so experimentalDecorators/emitDecoratorMetadata inherited from a shared config actually reach the transpiler. Add decorator-specific regression tests covering each extends form.
…e parent Switch TSConfigJSON.emit_decorator_metadata and .experimental_decorators to ?bool so 'not specified' is distinguishable from 'explicitly false', and change the extends merge to child-overrides-parent instead of OR. Skip the '<subpath>/tsconfig.json' probe when the subpath already ends in .json.
Keeps parity with the relative-extends branch so invalid specifiers like '!!!bad!!!' still surface under logLevel=debug (fixes bun-run.test.ts). Also default preserve_imports_not_used_as_values to null so the extends merge treats 'unset' distinctly, and add a test that directly observes design:type metadata emission through an inherited emitDecoratorMetadata.
ENOENT/ENOTDIR/EISDIR stay silent (expected while probing candidate suffixes); anything else (EACCES, EIO, etc.) is surfaced as a debug log with the specific path, so a file that exists but can't be read doesn't silently fall through to a different node_modules ancestor.
…'remove' Now that the field defaults to null (unset) instead of false, the '.remove' parser arm needs to assign false explicitly so a child config with importsNotUsedAsValues:'remove' can override an inherited 'preserve'/'error'.
A bare package name (no subpath) always names a directory in node_modules, so trying to parse it as a file is a guaranteed EISDIR. Only run probe 1 when there is a subpath.
The extends value comes straight from tsconfig.json and can be arbitrarily long; joining it into the fixed 4K path buffer with absBuf could overflow. Use absBufChecked (same pattern as the node_modules import-path lookup) and skip to the parent directory on overflow.
parseTSConfig already dupes the path into dirname_store on success, so the pre-emptive append in tryParseTSConfigPath left an orphaned copy for every failed probe (and a redundant one on success). Pass the buffer-backed candidate directly; it stays valid for the synchronous parseTSConfig call and the debug-log format.
2941f5e to
2177316
Compare
3 participants
Summary
Fixes tsconfig.json
extendsresolution for bare package specifiers so that settings likeexperimentalDecorators/emitDecoratorMetadataare correctly inherited from shared configs innode_modules.Previously, Bun naively joined the
extendsvalue against the tsconfig's directory. For"extends": "@repo/typescript-config/tsconfig.json"this produced<project>/@repo/typescript-config/tsconfig.jsoninstead of looking innode_modules, so the extended config was silently ignored. Since ce715b5 (standard decorators by default), this meant frameworks like TypeORM / MikroORM / NestJS would crash withundefined is not an object (evaluating 'target.constructor')because TC39 standard decorator semantics were applied instead of legacy semantics.The resolver now detects package specifiers with
isPackagePath()and walks up throughnode_modules(matching TypeScript's lookup), handling:@scope/pkg/tsconfig.base.json— explicit subpath@scope/pkg/pkg— bare package name → implicit<pkg>/tsconfig.json@scope/pkg/base— extensionless subpath →<subpath>.json@scope/pkg/configs— subpath directory →<subpath>/tsconfig.jsonAlso adds the missing
experimentalDecoratorsmerge in the extends chain (onlyemitDecoratorMetadatawas merged before).Test plan
test/regression/issue/06326.test.tscovers each extends form with a property decorator that reports whether it was called with legacy (target= prototype) or standard (target=undefined) semantics:tsconfig.json)node_modulesin a parent directory (monorepo layout)All tests fail with the released bun and pass with this build. Existing decorator/tsconfig bundler tests (
es-decorators.test.ts,decorators.test.ts,decorator-metadata.test.ts,bundler_decorator_metadata.test.ts,esbuild/tsconfig.test.ts) pass unchanged.Closes #6326