Repository navigation
Conversation
PR #26436 added the experimental_decorators field on TSConfigJSON and wired up the parse/consume sites, but forgot the merge step in dirInfoUncached's extends-chain loop — emit_decorator_metadata is OR-merged on the line just above it, experimental_decorators is not. Any tsconfig that sets experimentalDecorators:true and also extends a parent that doesn't set it silently loses the flag, so Bun emits stage-3 decorators where legacy ones are expected (and reflect-metadata throws). Two drive-by fixes for --tsconfig-override while we're in here: - transpiler.configureLinker now falls back to enclosing_tsconfig_json when the top-level directory has no tsconfig.json of its own. The override attaches to the root-directory DirInfo (parent == null), children only see it through the enclosing pointer, so decorator defaults were read from a null tsconfig. - parseTSConfig for the override gets .invalid as its dirname_fd instead of the current directory's fd. The override path often lives somewhere that isn't a child of the dir we're iterating, which tripped the openat(dirname_fd, basename(path)) fallback in cache.zig and printed a bogus "Internal error: directory mismatch" warning.
|
Updated 3:52 AM PT - May 11th, 2026
❌ @robobun, your commit c00899a has 4 failures in
🧪 To try this PR locally: bunx bun-pr 30478That installs a local version of the PR into your bun-30478 --bun |
|
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:
WalkthroughMake decorator-related TSConfig fields nullable, change extends merge to per-key override semantics, avoid directory-FD mismatches for --tsconfig-override, wire merged optional flags into transpiler/runtime defaults, and add regression tests covering extends and override cases. ChangesTypeScript Config Merge and Override Handling
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Thanks bot, good catch. #27248 overlaps on the
Merging either should close #30477. If #27248 lands first, this PR still has the two |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/resolver/resolver.zig`:
- Around line 4254-4255: The OR-merge for merged_config.emit_decorator_metadata
and merged_config.experimental_decorators makes a parent true sticky; change
these fields to an optional/tri-state (e.g., ?bool or a presence flag) and
update the merge logic so the merged value is: use the child's explicit value
when present (even if false), otherwise inherit the parent's value, rather than
OR-ing; adjust any places that read these flags to treat "unset" as nil/absent
and to interpret the resolved
merged_config.<emit_decorator_metadata|experimental_decorators> after merging.
In `@test/regression/issue/30477.test.ts`:
- Around line 1-8: The current multi-line prose header should be replaced with
the repo's two-line regression header: change the comment block at the top of
the test to two lines — first line the full GitHub issue URL (e.g. //
https://github.com/oven-sh/bun/issues/30477) and second line a single short
description of the bug (e.g. // experimentalDecorators lost when tsconfig
extends another), removing the extra explanatory lines so the file follows the
standard pattern used in test/regression/issue/.
🪄 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: 2593ae6b-f433-4fb2-95e3-b875c662118c
📒 Files selected for processing (3)
src/bundler/transpiler.zigsrc/resolver/resolver.zigtest/regression/issue/30477.test.ts
Address review: the OR-merge made parent `true` sticky, so a child tsconfig could not turn off `experimentalDecorators` or `emitDecoratorMetadata` inherited from its base. TypeScript's `extends` semantics are per-key override — a child's explicit value wins, even when it's `false`. Change the two fields on TSConfigJSON from `bool` to `?bool` so the merge loop can distinguish "child didn't say" (null) from "child explicitly set false". Consumers (resolver.dirInfoCached, transpiler.configureLinker, JSTranspiler) unwrap with `orelse false`. Two new test cases pin down the override semantics: - child `experimentalDecorators: false` overrides parent `true` (base enables legacy decorators, child disables them → stage-3 emit). - child `emitDecoratorMetadata: false` overrides parent `true` (bundler output must not contain `__legacyMetadataTS` calls).
- Tests use test.concurrent — each runs in its own tempDir with no shared state, so they can overlap (2.9s vs 5.6s sequential). - runEnvLoader in transpiler.zig uses the same enclosing_tsconfig_json fallback as configureLinkerWithAutoJSX, for consistency across the two top-level-dir lookups. Practically a no-op for the --tsconfig-override path (configureLinker already assigns options.jsx fully before this runs), but keeps the two call sites in sync.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/regression/issue/30477.test.ts`:
- Around line 144-150: Move the stdout assertion before the exit code assertion
so failures show actual output: place
expect(stdout).not.toContain("__legacyMetadataTS") before
expect(exitCode).toBe(0); keep the existing stderr logging (the if (exitCode !==
0) { console.log("stderr:", stderr); }) in place so stderr is still printed on
non-zero exits, but perform the stdout check prior to asserting exitCode.
🪄 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: fdd4bc41-6015-4d2b-af92-7cb82c5742b6
📒 Files selected for processing (5)
src/bundler/transpiler.zigsrc/resolver/resolver.zigsrc/resolver/tsconfig_json.zigsrc/runtime/api/JSTranspiler.zigtest/regression/issue/30477.test.ts
- Tests 1-3 now assert `stderr === ""` so an unexpected resolver or
transpiler warning on the non-override extends-chain paths fails the
test instead of passing silently. Test 3 (override) keeps its
targeted `not.toContain("directory mismatch")` — more specific than
strict-equality for that regression.
- Bundler test asserts stdout before exitCode so a mismatch surfaces
the actual build output in the failure diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/regression/issue/30477.test.ts`:
- Around line 46-48: Replace the unconditional stderr assertions in the test
(the expect(stdout).toBe("legacy\nOK\n"); expect(stderr).toBe("");
expect(exitCode).toBe(0); block and the similar blocks at the other noted
locations) with the conditional-on-failure pattern used later in the file: keep
the stdout and exitCode assertions, but only assert on stderr when the child
failed (e.g., if (exitCode !== 0) { expect(stderr).toBe(/* expected error output
*/); }). Update the three unconditional expect(stderr).toBe("") calls to follow
this conditional pattern so ASAN warnings on stderr won't cause flaky failures.
🪄 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: b4a39b5b-a7fd-4944-86f8-1639b3751261
📒 Files selected for processing (1)
test/regression/issue/30477.test.ts
…strict empty check)
Windows CI fails the strict `expect(stderr).toBe("")` — the test/
runtime on that platform emits harmless informational lines to stderr
that we don't want to gate the assertion on. Switch to the
conditional-on-failure pattern documented in test/CLAUDE.md: only
pin stderr when the exit code is non-zero, which still surfaces the
full stderr in the failure diff without making the green path
flaky.
Previously used `bun build --target bun` + a grep for `__legacyMetadataTS` in the bundled output. That helper-function name is an implementation detail of the runtime — if Bun ever renames or inlines it the test breaks silently. Switch to a pure-runtime check: the test script installs a `Reflect.metadata` shim that records the class/key pair into a WeakMap, then prints whether the map has an entry for `Foo.prototype`. With `emitDecoratorMetadata: false` (child override), the shim is never invoked and the map stays empty; with the baseline-buggy OR-merge behavior, base's `true` wins and the shim fires. That means this case still fails against system bun and still passes against the fix, but doesn't depend on any emitted identifier name.
…ts on 2019 x64 shard 3/8
|
Diff is green; CI failures on this PR were on unrelated flaky lanes only, none touching the resolver/tsconfig/transpiler code here:
Used my one re-roll on build 53270. Handing off — needs a maintainer to merge. |
|
the test cases look solid |
Tried bumping to 1.18.1, but the bundled Bun (1.3.13) emits TC39 standard decorators from Bun.build() regardless of experimentalDecorators in tsconfig, breaking Lit's legacy @property/@state/@query/@consume. A TC39 migration is non-trivial (accessor keyword everywhere, plus subtle @query/@consume initialization-order issues we couldn't tame in one pass). Stay on 1.16.0 until oven-sh/bun#30478 lands. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Closing: this PR's implementation lives entirely in Zig source files that have since been removed from the tree as part of the Rust migration. The change can no longer merge cleanly and the files it edits no longer exist on If the underlying issue is still present, it will need a fresh fix against the Rust implementation. |
Electrobun 2.x replaces the CLI with Hutch and bundles through Cottontail, which honours experimentalDecorators — the built view bundle emits __legacyDecorateClassTS, so Lit's decorators work and the 1.16.0 pin from ADR-0001 is retired. Its unblock condition never could have been met: oven-sh/bun#30478 was closed unmerged when the Rust migration deleted the files it patched. The main process stays Bun (bun:ffi, Bun.serve), which electrobun pins at 1.4.0 and packages itself. Details in ADR-0017. The SDK now lives in a generated .hutch/devkit rather than node_modules, so tsconfig maps electrobun/* there, CI syncs before typecheck, and @types/three — only ever needed for electrobun 1.16's untyped three import — is dropped. The config's tsconfig-paths plugin goes with it: 2.x serializes the config, and Cottontail's bundler reads tsconfig paths itself. maplibre-gl 6 is ESM-only and removed the internal map.transform. The custom points layer reads its projection data off the render input instead, and the popup's hand-rolled globe silhouette mask is replaced by maplibre's own locationOccludedOpacity — a popup behind the globe now fades rather than being clipped. GeoJSONSource.setData returns a promise in 6, marked void at the twelve fire-and-forget call sites. The bundle now carries import.meta.url, so index.html loads it as a module and the worker ships beside it. Also: prettier 3.9, eslint 10.9 + eslint-config-love 155, @lit-labs/signals 0.3, playwright 1.62 (run e2e:install once), and patch bumps. TypeScript 7 is skipped — typescript-eslint refuses to load against it, and running lint on a second, separate TS 6 compiler would leave lint and typecheck disagreeing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019u22Bj8GxveBJkHLwxHpy8
Fixes #30477.
Problem
A tsconfig that sets
experimentalDecorators: trueandextendsa parent that does not set it silently loses the flag:Bun emits stage-3 (TC39) decorators instead of legacy ones, so
reflect-metadatathrowsTypeErrorand real-world decorator codebases break on 1.3.10+. Worked in 1.3.5–1.3.9.Root cause
PR #26436 introduced the TC39 decorators pipeline and added
TSConfigJSON.experimental_decorators— parsed attsconfig_json.zig:182, consumed atresolver.zig:1067— but did not add the corresponding merge in the extends-chain loop atresolver.zig:4253. The neighbouringemit_decorator_metadatais OR-merged on the line just above;experimental_decoratorswas missed. Since the loop'smerged_configstarts as the base config (experimentalDecorators: falseby default) and the child's value never gets OR'd in, the child'strueis discarded.Fix
One-line merge matching the pattern already used for
emit_decorator_metadata:Drive-by fixes for
--tsconfig-overrideThe reproducer in #30477 also uses
--tsconfig-override ./tsconfig.bun.json, which surfaced two adjacent bugs:Internal error: directory mismatchwarning —parseTSConfigwas called with the directory-fd of the dir being iterated, but the override path may live outside that dir.openat(dirname_fd, basename(path))incache.zigfails with ENOENT and falls back to an absolute-path open, logging the warning along the way. Fix: pass.invalidwhen loading an override at the root-directory slot.Override ignored for decorator defaults — the override attaches to the filesystem root (
parent == nullindirInfoUncached). Children inherit it viaenclosing_tsconfig_json, nottsconfig_json.transpiler.configureLinkeronly checkedroot_dir.tsconfig_json, so it readnullfor the working directory and leftexperimental_decorators/emit_decorator_metadataunset. Fix: fall back toenclosing_tsconfig_json.This second fix also benefits the plain case where a tsconfig lives in an ancestor directory (also previously silently ignored by the transpiler's defaults).
Verification
test/regression/issue/30477.test.tscovers:experimentalDecorators: truein the child tsconfig with anextendschainexperimentalDecorators: trueinherited from the base config--tsconfig-overridewith an extends chain (both the decorator behaviour and the absence of the "directory mismatch" warning)Existing related tests all still pass (40 tests across
decorators.test.ts,es-decorators.test.ts,tsconfig-override.test.ts,#27575,#27526,transpiler-tsconfig-uaf.test.ts).