Keep escaped newlines in string literals without --minify-syntax#32577
Keep escaped newlines in string literals without --minify-syntax#32577robobun wants to merge 3 commits into
Conversation
bun build converted any string literal containing a \n escape into a backtick template with a literal newline, even with no minify flag. best_quote_char_for_string counted a newline as a cost against single and double quotes but not backticks, so a newline always tipped the quote choice toward a template literal. Gate that newline cost on minify_syntax. Without --minify-syntax the newline now stays escaped inside a quoted string, matching esbuild; --minify-syntax still produces the template literal.
|
Updated 1:16 PM PT - Jun 21st, 2026
❌ @robobun, your commit 9343b96 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 32577That installs a local version of the PR into your bun-32577 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe JS printer's ChangesPrinter newline-to-template-literal fix
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
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 `@src/js_printer/lib.rs`:
- Around line 828-836: The issue is that backtick_cost is not being penalized in
non-minify mode when newlines are present, allowing backticks to be selected
over quoted strings even when minify_syntax is false. To fix this, you need to
also increase the backtick_cost when minify_syntax is false in the same
conditional block where single_cost and double_cost are being incremented. This
ensures that in non-minify mode, escaped newlines in quoted strings are
preserved instead of allowing backticks to win and emit literal newlines via
template literals. Modify the if statement that currently only updates
single_cost and double_cost to also update backtick_cost when minify_syntax is
false.
🪄 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: 1746a2c3-4228-4eeb-90db-60b1bc226d1e
📒 Files selected for processing (3)
src/js_printer/lib.rstest/bundler/bundler_string.test.tstest/bundler/transpiler/transpiler.test.js
A string containing both ' and " selects a backtick to avoid escaping either quote, independent of minify_syntax and of any newline. esbuild emits the same, so assert it to document the behavior.
There was a problem hiding this comment.
I didn't find any issues with the change itself — it's a small, well-tested gate that matches esbuild — but CI reported build-rust failures on b183590 across all platforms and the follow-up commit only touches tests, so worth a human confirming CI is green before merging.
Extended reasoning...
Overview
This PR threads a minify_syntax: bool parameter through best_quote_char_for_string / best_quote_char_for_e_string in src/js_printer/lib.rs and gates the newline cost increment on it. Five call sites are updated to pass self.options.minify_syntax. Six new bundler tests in bundler_string.test.ts cover no-minify, --minify-whitespace, --minify-syntax, and the both-quotes / double-quote-with-newline edge cases; one transpiler assertion is updated to the new escaped form. All callers of the changed function are accounted for in the diff.
Security risks
None. This is purely a quote-character selection heuristic in the JS printer — no auth, crypto, filesystem, or network surface. The emitted string value is semantically identical regardless of quote choice; only the textual representation changes.
Level of scrutiny
Moderate. The logic change is tiny (wrap two += 1 lines in an if), mechanical, and verified against esbuild as the reference. CodeRabbit's one concern (both-quotes + newline) was checked against esbuild, found to already match, and a regression test was added; CodeRabbit withdrew the comment. However, this is the core JS printer and the change alters user-visible bun build output for every string literal containing \n in non-minified mode.
Other factors
The robobun CI comment reports build-rust failures on commit b183590 across all Linux/FreeBSD targets. The follow-up commit c1400b5 only adds a test case, so it would not resolve a Rust compile error. The Rust diff itself looks syntactically sound, so this may be transient CI flakiness — but I can't confirm CI is green from here, and that's the main reason I'm deferring rather than approving.
|
Thanks for flagging. The The current run on the latest commit (c1400b5, build #63825) has For what it's worth, |
There was a problem hiding this comment.
LGTM — focused fix that gates the newline→backtick bias on --minify-syntax, matches esbuild, and is well covered by the new tests.
Extended reasoning...
Overview
This PR threads a minify_syntax: bool parameter through best_quote_char_for_string / best_quote_char_for_e_string in src/js_printer/lib.rs and gates the newline cost increment on it. Without --minify-syntax, a \n in a string literal no longer biases quote selection toward a backtick template literal, so the escape is preserved in the quoted output. All six call sites are updated to forward self.options.minify_syntax. Tests are added in test/bundler/bundler_string.test.ts (six cases covering no-minify, --minify-whitespace, --minify-syntax, both-quotes, both-quotes-with-newline, and double-quote-with-newline) and one expectation in test/bundler/transpiler/transpiler.test.js is corrected.
Security risks
None. This only affects which quote character the printer picks for a string literal; the emitted JavaScript is semantically identical either way ("Hello\nWorld" vs a backtick with a literal newline). No auth, crypto, permissions, or data handling is touched.
Level of scrutiny
Low–medium. While js_printer is production code that affects all bun build output, the change is a narrow heuristic tweak inside a single match arm: wrap two += 1 increments in if minify_syntax { ... }. The remaining cost calculations (for ', ", `, ${) are untouched, so pre-existing quote selection (e.g., backtick when both quote chars are present) is preserved. Two of the updated call sites pass allow_backtick: false anyway, making the new parameter a no-op there. The worst-case failure mode is a different-but-still-valid quote character, not broken output.
Other factors
- Fixes #32576 and matches esbuild's reference behavior, which the author verified across no-flags /
--minify-whitespace/--minify-syntax. - The CodeRabbit edge case (both quotes + newline without minify) was investigated, shown to match esbuild, and locked in with the
BothQuotesWithNewlineUseBacktickWithoutMinifyregression test; that thread is resolved. - No CODEOWNERS cover
src/js_printer/. - The bug-hunting system found no issues, and CI
build-rustis green across all platforms per the latest status.
CI status: diff is green, failing lanes are known flakesGreen on this PR: This change only selects a different quote character for newline-containing string literals and preserves the string value, so it has no path to TLS, S3, the shell, the V8 heap snapshot, or the installer. The red lanes are pre-existing flaky tests: they vary between my two runs, and the recurring one also fails on unrelated builds.
Base commit 602d956 passed full CI (build 63771). I've already re-run CI once; this needs a maintainer to merge or re-trigger. |
Fixes #32576
Problem
bun buildconverts any string literal containing a\nescape into a backtick template literal with a literal newline, even with no minify flag:The template-literal conversion is documented as a minifier feature, so it should not happen unless syntax is being minified.
Cause
best_quote_char_for_stringinsrc/js_printer/lib.rscounts a newline as a cost against single and double quotes but not against backticks. Withbacktick_costleft at 0, any newline-containing string satisfiesbacktick_cost < single_cost.min(double_cost)and the quote choice becomes a backtick. This ran unconditionally, independent of the minify options.Fix
Gate the newline cost on
minify_syntax(threaded throughbest_quote_char_for_string/best_quote_char_for_e_stringand their callers). Without--minify-syntaxa newline no longer biases toward a backtick, so it stays escaped inside a quoted string.--minify-syntax(and full--minify) keep the existing template-literal output.This matches esbuild, the reference implementation:
"Hello\nWorld""Hello\nWorld""Hello\nWorld"--minify-whitespace"Hello\nWorld""Hello\nWorld"--minify-syntaxThe newline cost only affected the backtick decision, so strings with both
'and"(no newline) still choose a backtick to avoid escaping, matching esbuild; a string with a"and a newline now picks a single quote with an escaped\n, also matching esbuild.Verification
Tests in
test/bundler/bundler_string.test.tscover the no-minify,--minify-whitespace, and--minify-syntaxpaths plus the both-quotes and double-quote-with-newline variants. The existing quote-selection assertion intest/bundler/transpiler/transpiler.test.jsis updated to the escaped form. New tests fail on the current build and pass with the fix.