Repository navigation
Conversation
emitDecoratorMetadata alone (or with experimentalDecorators:false) was selecting the legacy decorator lowering. tsc treats emitDecoratorMetadata as a no-op without experimentalDecorators and emits standard decorators, so a stage-3 decorator would silently receive the legacy (target, key, descriptor) calling convention under bun. standard_decorators is now keyed on experimentalDecorators alone, and emit_decorator_metadata is gated on it. Updates the #27526 regression test to set experimentalDecorators explicitly, matching what tsc and NestJS require.
WalkthroughChangesThe parser and transpiler now enable legacy decorator metadata only when both decorator options are set. Tests cover standard decorators without experimental mode and legacy decorators with metadata enabled. Decorator mode selection
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 3:10 PM PT - Jul 27th, 2026
❌ @autofix-ci[bot], your commit ac4df34 has some failures in 🧪 To try this PR locally: bunx bun-pr 36142That installs a local version of the PR into your bun-36142 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![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 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/27526.test.ts`:
- Around line 4-6: Replace the explanatory comment in the regression test with
only the issue URL comment for Bun issue 27526, preserving the surrounding test
code unchanged.
🪄 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: a522b8a6-3df6-4674-a6bf-23c3386b1371
📒 Files selected for processing (4)
src/bundler/ParseTask.rssrc/bundler/transpiler.rstest/bundler/transpiler/es-decorators.test.tstest/regression/issue/27526.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
… bundler path The extends merge loop OR-merged emit_decorator_metadata but not experimental_decorators, so a child tsconfig setting experimentalDecorators: true over a base that does not was dropped. Previously masked by the emit_decorator_metadata fallback in the standard_decorators computation. Also adds itBundled coverage for the ParseTask.rs path.
There was a problem hiding this comment.
The two earlier findings (resolver extends merge and bundler-path coverage) are addressed and the remaining OR-merge limitation is a pre-existing pattern acknowledged as follow-up. I'm deferring rather than approving because this deliberately reverses the #27527 heuristic — projects with emitDecoratorMetadata: true and no experimentalDecorators flip from legacy to standard decorators, and the #27526 regression test's contract is changed to require the flag. That's a defensible tsc-alignment call, but it's a user-facing breaking change a maintainer should sign off on.
What was reviewed:
standard_decorators/emit_decorator_metadatagating in bothtranspiler.rsandParseTask.rs— kept in lockstep, both now covered by tests.resolver.rsextendsmerge —experimental_decoratorsnow OR-merged alongsideemit_decorator_metadata; child-overrides-base=false limitation is pre-existing and unchanged.- Confirmed no other
src/sites derivestandard_decoratorsfrom these flags.
Extended reasoning...
Overview
Changes the decorator-flavor selection in the runtime transpiler (src/bundler/transpiler.rs) and the bundler parse worker (src/bundler/ParseTask.rs) so that legacy TypeScript decorators are selected only when experimentalDecorators is set, and emitDecoratorMetadata is gated on the same flag (matching tsc's TS5052 semantics). Adds an OR-merge for experimental_decorators in the tsconfig extends loop in src/resolver/resolver.rs. Nine new tests across bun run, Bun.Transpiler, and Bun.build pin both directions plus the extends chain; the #27526 regression test is updated to set experimentalDecorators: true explicitly.
Security risks
None. Pure transpiler-option plumbing; no untrusted input parsing, auth, or crypto paths touched.
Level of scrutiny
Medium-high. The code change is small and mechanically correct against tsc semantics, and both prior review findings (missing experimental_decorators merge in the extends loop; missing itBundled coverage for ParseTask.rs) were fixed in f7c90da. The remaining 🟡 note — child experimentalDecorators: false cannot override a base's true because the field is a plain bool — is a pre-existing limitation shared with the adjacent emit_decorator_metadata merge and reasonably deferred.
Other factors
The reason to defer is not correctness but the product decision: the original || emit_decorator_metadata heuristic was added in #27527 specifically so NestJS-style projects that set only emitDecoratorMetadata would get legacy semantics. This PR reverses that and rewrites the #27526 regression test to require the flag, with the PR body noting affected projects "will need to add that flag to their tsconfig." REVIEW.md flags both "never silently weaken an existing test" and "never change a Bun-native default" as things a maintainer should explicitly agree to. The change is well-argued (tsc alignment, NestJS docs already require both flags), but whether to accept the ecosystem breakage is a maintainer call, not a bot call.
|
CI build #83601: the gate (
The decorator tests added in this PR ( |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-27, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
2 participants
Repro
Before:
After (matches tsc):
At runtime, with tsconfig
{ experimentalDecorators: false, emitDecoratorMetadata: true }a method decorator was being called as(target, key, descriptor)(legacy) instead of(value, context)(standard), so stage-3 decorators silently received the wrong arguments with no diagnostic.Cause
src/bundler/transpiler.rsandsrc/bundler/ParseTask.rscomputedstandard_decorators = !loader.is_typescript() || !(experimental_decorators || emit_decorator_metadata), originally added in #27527 as a heuristic for NestJS projects that setemitDecoratorMetadatawithoutexperimentalDecorators. tsc does not do this:emitDecoratorMetadatais a no-op unlessexperimentalDecoratorsis also set (TS5052), and standard decorators are emitted.Fix
Key
standard_decoratorsonexperimentalDecoratorsalone, and gateemit_decorator_metadataon it as well so the metadata flag remains a no-op in the standard path.This unmasked a pre-existing gap in the tsconfig
extendsmerge:resolver.rsOR-mergedemit_decorator_metadataacross the chain but never mergedexperimental_decorators, so a child tsconfig settingexperimentalDecorators: trueover a base that does not was dropped. That was previously hidden by the|| emit_decorator_metadatafallback. The merge now copiesexperimental_decoratorsalongsideemit_decorator_metadata.The #27526 regression test is updated to set
experimentalDecorators: trueexplicitly, which is what tsc (and the NestJS docs) require. Projects that relied on the old heuristic will need to add that flag to their tsconfig.Verification
test/bundler/transpiler/es-decorators.test.ts:bun runandBun.Transpilerfor{ emitDecoratorMetadata: true }alone and{ experimentalDecorators: false, emitDecoratorMetadata: true }; a guard that both flags together still emit__legacyDecorateClassTSwithdesign:*metadata; and twoextends-chain tests (flag in child, flag in base) pinning legacy semantics.test/bundler/bundler_decorator_metadata.test.ts: twoitBundledcases covering theBun.buildpath for the same tsconfigs.Six of the nine new cases fail on main.
[review] gate passed · iteration 0 · 6 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file