Skip to content

macros: inline a text Response or Blob as its text, not as its JSON-escaped form - #46680

Open
robobun wants to merge 1 commit into
mainfrom
robobun/46bf8d14/macro-text-blob
Open

robobun wants to merge 1 commit into
mainfrom
robobun/46bf8d14/macro-text-blob

Conversation

@robobun

@robobun robobun commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • A text Response or Blob that a macro returns is inlined as its JSON-escaped form: a newline becomes the two characters \n, and é becomes \u00E9. The documented return fetch(url) of a text/* body is affected.
  • Regression in v1.1.35. v1.1.34 inlines the text.
  • expr_from_blob (src/js_parser_jsc/Macro.rs:1171) stores the quote_for_json form of the bytes as an 8-bit string, and the printer escapes an 8-bit string again.

Fix

Background

Downsides

  • The output goes back to that of v1.1.34. A build that un-escapes the inlined text by hand now un-escapes real text.
  • The release binary grows by 256 bytes of text. A text body costs 2 allocator calls (6 on main).
Notes

Repro

// m.ts
export function text() {
  return new Response("a\nb \"q\" \\ é", { headers: { "content-type": "text/plain" } });
}
export function blob() {
  return new Blob(["a\nb \"q\" \\ é"], { type: "text/plain" });
}
// index.ts
import { text, blob } from "./m.ts" with { type: "macro" };
console.log(JSON.stringify([text(), blob()]));
build (linux x64, release) bun index.ts prints
v1.1.34 ["a\nb \"q\" \\ é","a\nb \"q\" \\ é"]
v1.1.35 ["a\\nb \\\"q\\\" \\\\ \\u00E9","a\\nb \\\"q\\\" \\\\ \\u00E9"]
main 6292c65 same as v1.1.35
this PR same as v1.1.34

I ran the two old releases to find the version. I did not bisect the commit. #15009 is in that range and changed how the printer writes strings.

What the inlined string is now. The test compares six macro results with the string blob.text() gives:

  • a body with a newline, quotes, a backslash, a tab, a NUL, é and an emoji, as a Blob, as a Response, and fetched from a local server
  • the same body after a UTF-8 byte-order mark
  • the same body as UTF-16LE after its byte-order mark
  • a malformed UTF-8 byte, which becomes U+FFFD

Measurements (release, main 6292c65 against this PR, linux x64):

  • llvm-size: text 107,688,070 to 107,688,326. expr_from_blob 2,493 to 2,602 bytes (blob_text is inlined into it).
  • Allocator calls inside expr_from_blob per macro-returned text Blob, counted with gdb breakpoints on the mimalloc entry points over 101 call sites: ASCII 6 to 2, non-ASCII 6 to 3, empty 4 to 2.
  • Bytes that stay in the arena for a 16-byte body: ASCII 56 to 56. With one é: 61 to 72, because the text is UTF-16 now and was the escaped bytes before.
  • A JSON or binary body: no change.

Suites run on the debug build: test/bundler/transpiler/macro-test.test.ts (35 pass).

Related

The text arm of expr_from_blob stored the body as a JSON-escaped string,
and the printer escapes an 8-bit string again. Since v1.1.35 a body with
a newline, a quote, a backslash or a non-ASCII character was inlined as
its escaped form: "a\nb" became "a\\nb".

Store the string that blob.text() returns for the bytes. A UTF-8
byte-order mark is dropped, a UTF-16LE one selects that encoding, and
non-ASCII text is stored as UTF-16, like a literal from the lexer.
@robobun

robobun commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

Status: ready for review.

How to reproduce. Use a build without this change.

// m.ts
export function text() {
  return new Response("a\nb \"q\" \\ é", { headers: { "content-type": "text/plain" } });
}
// index.ts
import { text } from "./m.ts" with { type: "macro" };
console.log(JSON.stringify(text()));
bun index.ts
  • v1.1.34: "a\nb \"q\" \\ é"
  • v1.1.35 and main 6292c65: "a\\nb \\\"q\\\" \\\\ \\u00E9"
  • With this change: "a\nb \"q\" \\ é"

@robobun

robobun commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 11:26 PM PT - Oct 23rd, 2026

✅ @robobun, your commit f7cc50a40b710c0f78d4e8d88dab2b576ffeab40 passed in Build #131646! 🎉


🧪   To try this PR locally:

bunx bun-pr 46680

That installs a local version of the PR into your bun-46680 executable, so you can run:

bun-46680 --bun

@coderabbitai

coderabbitai Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 7d240c1 and f7cc50a.

Files selected for processing: 2
  • src/js_parser_jsc/Macro.rs
  • test/bundler/transpiler/macro-test.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.

Walkthrough

Text-like Blob content now uses blob_text to create macro strings. The added end-to-end test covers Blob and Response text, BOM handling, malformed UTF-8, and successful process completion.

Changes

Macro Blob text decoding

Layer / File(s) Summary
Decode text-like Blob content
src/js_parser_jsc/Macro.rs, test/bundler/transpiler/macro-test.test.ts
expr_from_blob uses blob_text to create macro strings. The new test checks text from Blobs, Responses, and fetch(), including BOM handling and malformed UTF-8. It also checks empty stderr and a zero exit code.

Suggested reviewers: jarred-sumner, dylan-conway

Priority: Normal

Merge Risk: Minimal · up to f7cc5

The text-decoding change has no identified issue requiring a fix before merge.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: macros now inline text Responses and Blobs as text instead of JSON-escaped content.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The description explains the problem, fix, scope, verification results, regression context, tests, performance impact, and related changes. It does not use the template headings exactly, but it provid…
Full details: Description check

Explanation

The description explains the problem, fix, scope, verification results, regression context, tests, performance impact, and related changes. It does not use the template headings exactly, but it provides the required information in equivalent sections.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline finding, I also checked: the UTF-16LE odd-trailing-byte drop and the lone-BOM edge (BOM::detect needs 3 bytes) match Blob::to_string_with_bytes in src/runtime/webcore/Blob.rs, which uses the same BOM::detect_and_split and len & !1 truncation, so blob_text is consistent with what .text() returns; an is_utf16 E::String from a macro is not a new shape, the T::String arm in Macro.rs already builds one via init_utf16; and all three branches copy into the bump arena, so nothing borrows the Blob's store past the call.

Extended reasoning...

The change replaces the double-escaping text path in expr_from_blob with a blob_text helper mirroring Blob.text() (BOM handling, ASCII as 8-bit, otherwise UTF-16 via init_re_encode_utf8) and adds one subprocess test covering Blob, Response, local fetch, both BOMs and malformed UTF-8. It touches no security-sensitive surface. The parity with Blob.rs and the existing UTF-16 macro-string shape rule out the encoding-edge concerns listed; the one confirmed finding is posted inline.

Comment thread src/js_parser_jsc/Macro.rs Resolved

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants