Repository navigation
minify-syntax: keep this for ns["tag"]x on a lifted CommonJS export - #42549
Conversation
…["b"] to a.b The rewrite moved call_target and delete_target to the new E::Dot, but not template_tag. So ns["tag"]`x` on a lifted CommonJS export printed as $tag`x`, and the tag function ran with this === undefined.
|
Updated 7:57 PM PT - Sep 12th, 2026
✅ @robobun, your commit c4fb19213088f57c3638d409e47f857d9ced4077 passed in 🧪 To try this PR locally: bunx bun-pr 42549That installs a local version of the PR into your bun-42549 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughThe visitor now preserves template-tag tracking when it converts computed properties to dot access. Bundler tests cover imported methods, template tags, and self-referential ChangesComputed member minification
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The fix preserves receiver binding for minified computed template tags, with regression coverage for import and CommonJS export paths. No actionable merge risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the is_template_tag local was already computed at the top of e_index and this just propagates it to the rewritten dot node, exactly mirroring the neighboring is_call_target/is_delete_target arms and esbuild's p.templateTag = dot.
What was reviewed:
- Confirmed
is_template_tagis computed at line 854 alongside the other two markers, so the added block is the missing sibling, not a new code path. - Checked the second
a["x"+"y"]→a.brewrite further down ine_index— it returns without re-visiting, so no marker propagation is needed there (matches the PR's claim). - Tests reuse the existing
thisReadingLibfixture, cover all three consumers (default/star/self), and assert both emitted bundle text and runtime stdout; the star-import case correctly avoids pinning the namespace variable name.
Extended reasoning...
Overview
This PR is a three-line fix in src/js_parser/visit/visit_expr.rs (e_index), plus three new itBundled test cases appended to test/bundler/bundler_cjs2esm.test.ts. When --minify-syntax rewrites X["name"] to X.name, the visitor already re-tags the replacement E::Dot node as p.call_target and p.delete_target; the fix adds the identical treatment for p.template_tag. The is_template_tag boolean was already being computed at the top of the function (line 854) — it just wasn't being used at this rewrite site. Without the fix, ns["tag"]`z` on a lifted CommonJS export unwraps to a bare identifier and loses its this receiver.
Security risks
None. This is a bundler AST-marker propagation fix with no bearing on auth, crypto, permissions, network, filesystem, or untrusted input parsing. The change only affects which node address is stored in a parser-internal pointer field during expression visiting.
Level of scrutiny
Low. The fix is mechanical and byte-for-byte parallel to the two adjacent if blocks for call_target and delete_target. The PR description notes esbuild does the same (p.templateTag = dot), and REVIEW.md treats esbuild as the reference implementation for ported bundler logic. I verified the second index→dot rewrite in e_index (constant-folded string keys) does not re-visit the node and so needs no marker — the PR author's rationale checks out against the code. No CODEOWNERS entry covers src/js_parser/ or test/bundler/.
Other factors
The three new tests are placed in the existing bundler_cjs2esm.test.ts file (per CLAUDE.md guidance), reuse the file's shared thisReadingLib fixture, and assert both the emitted bundle text (toContain/toMatch on the exact console.log(...) line) and runtime stdout — so each test can fail on the code shape and on behavior independently. Each fixture uses only computed member forms (X["tag"]) so the test isolates the rewrite path rather than being satisfied by a sibling X.tag marking the symbol. The star-import test uses \w+ for the namespace name to avoid coupling to an unrelated open PR that renames it. The bug-hunting run exited on dry_streak with no findings and no ruled-out candidates. The PR timeline has no outstanding reviewer objections.
2 participants
Problem
bun build --minify-syntax,ns["tag"]`z`prints as$tag`z`whentagis a lifted CommonJS export. The tag then runs withthis === undefined:TypeError: undefined is not an object (evaluating 'this._helper').st["tag"]`z`on a default import andexports["tag"]`z`inside the CommonJS file break too.a["b"]toa.brewrite ine_index(src/js_parser/visit/visit_expr.rs:858). It movesp.call_targetandp.delete_targetto the newE::Dot, then visits it. It does not movep.template_tag(added in bundler: keep this for a method call on a lifted CommonJS export #41251). Soe_dotsees no tag, and the linker binds the export directly.Fix
p.template_tagat the newE::Dot. esbuild does the same (p.templateTag = dot).e_index(a["x" + "y"]) is unchanged. It returns the newE::Dotwithout a visit, so nothing reads the marker. That form already works.test/bundler/bundler_cjs2esm.test.ts(3 new tests, all fail on main). Also 11 other bundler suites (see the notes).Background
exports.foo = xin a CommonJS file intovar $foo = xplus an ES export. The namespace object (exports_lib) stands in formodule.exports.X.name`z`passesXasthis, like a method call.$name`z`passesundefined. bundler: keep this for a method call on a lifted CommonJS export #41251 makes the linker keepX.nameas written when the file calls it or tags a template with it.e_templatestores the tag node inp.template_tag.e_dotande_indexcompare their own node with it by address. A rewrite that replaces the node must move the marker.Notes
Repro (fails on 1.4.3-canary.1+6a92015fc and on main b993710).
git tag --contains f31440d21dlists bun-v1.4.1 and bun-v1.4.2, so both releases have the same code:out/min.json main holdsconsole.log($tag`z`). With this change it holdsconsole.log(exports_lib.tag`z`)and printshelped:z.The marker reaches three consumers, all through
IdentifierOpts::is_template_tagset ine_dot:maybe_rewrite_property_access(src/js_parser/fold.rs:271) forimport * as ns.record_import_property_use(src/js_parser/p.rs:6712) for a default import.note_commonjs_export_use(src/js_parser/p.rs:5955) forexports.namein the lifted file itself.Each new test covers one consumer. Each test also has the computed method call (
X["parse"]("y")), which already worked, so both markers are covered underminifySyntax. The nametagappears only in computed form in each entry, because one marked use of a name keeps everyX.nameof that file as written and would hide the fault.The second rewrite (
ns["ta" + "g"], or an inlined TypeScript enum key) does not go throughmaybe_rewrite_property_accessat all. It printsexports_lib.tag`z`on main and with this change. In esbuild that rewrite happens aftermaybeRewritePropertyAccessand carries no marker.Checked by hand with
--minify-syntaxand--minify, all printhelped:z:const ns = await import("./lib.cjs"),require("./lib.cjs")["tag"],const ns = require("./lib.cjs"),import * as nsthrough anexport *barrel, andimport { default as st }.Sites this change leaves alone:
e_binary, the ternary fold ine_if, and the[x][0]fold ine_index. They turn(0, o.f)`x`intoo.f`x`. That is the opposite fault (the tag gains a receiver). It also reaches plainbun run:(0, o.f)`x`seesthis === othere, andundefinedin node. bundler: wrapper folds drop(0, x.f)()for CommonJS import items,exports.f, template tags and one ternary path #42452 (section 4) tracks them.thisundefined for call targets and template tags that a rewrite turned into a property access #40829 (open, conflicts with main) is the candidate fix for those folds. Its diff has the same three lines as this change, as one hunk of 26 files. It predates bundler: keep this for a method call on a lifted CommonJS export #41251. If this change lands first, Keepthisundefined for call targets and template tags that a rewrite turned into a property access #40829 drops that hunk on its rebase.thisand getter for an unbound item of a require() / import() local #42402 (open) changeshas_value_for_this_in_callat the fold sites. It does not touch this rewrite orp.template_tag.The star import test matches the receiver with
\w+and does not pinexports_lib, because #41820 (open) givesimport *of a lifted module a namespace object of its own, with another name. The default import test and the self test pin the exact output.Suites run on the debug build:
bundler_cjs2esm,bundler_minify,bundler_cjs,bundler_edgecase,bundler_dynamic_import_dce,esbuild/default,esbuild/dce,esbuild/importstar,esbuild/importstar_ts,esbuild/ts,transpiler/transpiler,transpiler/macro-test.[human-review] gate passed · iteration 0 · 2 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