fix: preserve unicode in tagged template literals and regex source#27274
Open
fix: preserve unicode in tagged template literals and regex source#27274
Conversation
Changes `ascii_only` to `prefers_ascii` as a runtime parameter instead of comptime. When a tagged template literal or regex contains non-ASCII characters, the printer disables ASCII-only mode so the characters are emitted as-is rather than escaped. Also changes `cloneLatin1` to `cloneUTF8` in module loading so JSC correctly interprets the UTF-8 output. Fixes #8745, #15492, #15929, #16763, #18115, #8207 Co-Authored-By: pfg <pfg@pfg.pw> Co-Authored-By: Claude <noreply@anthropic.com>
Collaborator
Author
This comment was marked as spam.
This comment was marked as spam.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/8207/8207.test.ts`:
- Line 1: The regression test currently containing the import line "import {
expect, test } from \"bun:test\";" must be moved into the repository's canonical
regression issue folder and renamed to use the real issue number (8207.test.ts);
relocate the file accordingly, ensure its filename is exactly "8207.test.ts",
update any relative imports or references if they break after the move, and run
the test suite to verify it still executes.
In `@test/regression/issue/8745/8745.test.ts`:
- Line 1: This test file imports harness helpers but is missing the required
test runner imports; add an explicit import of test and expect from "bun:test"
at the top of the file (alongside the existing imports such as bunEnv, bunExe,
tempDirWithFiles) so the file uses bun:test's test() and expect() functions as
per the project guideline for *.test.* files.
- Line 9: Replace the large string created with `" ".repeat(16 * 1024 * 1024)`
in the test entry for "requires_rtc_fixture.ts" with a Buffer-based allocation
to avoid heavy string-repeat usage; use Buffer.alloc(16 * 1024 * 1024, '
').toString() (or equivalent Buffer.alloc(count, fill).toString()) so the test
creates the same repeated-space string but follows the guideline and is more
memory/CPU friendly.
Contributor
|
Newest first ✅ ca7ab — Looks good! Reviewed 11 files across Previous reviews✅ 35a03 — Looks good! Reviewed 11 files across |
- Add explicit `import { expect, test } from "bun:test"` to 8745.test.ts
Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/regression/issue/8745/8745.test.ts`:
- Line 10: Replace the use of string repetition for the large fixture with a
Buffer allocation: instead of `"requires_rtc_fixture.ts": fixture + "
".repeat(16 * 1024 * 1024)`, build the repeated-space string via Buffer.alloc(16
* 1024 * 1024, ' ').toString() and concatenate to fixture; update the entry for
"requires_rtc_fixture.ts" to use this Buffer.alloc-based string creation to
comply with the guideline.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Rebased version of #15047 by @pfgithub.
ascii_onlytoprefers_asciias a runtime parameter (instead of comptime) on the printer, so when a tagged template literal or regex contains non-ASCII characters, the printer can disable ASCII-only mode and emit them as-is rather than escaping themcloneLatin1tocloneUTF8in module loading paths so JSC correctly interprets the UTF-8 outputFixes #8745, fixes #15492, fixes #15929, fixes #16763, fixes #18115, fixes #8207
Test plan
test/regression/issue/8745/8745.test.ts- tagged template unicode (run directly, build, build+minify, with RTC)test/regression/issue/8207/8207.test.ts- regex source string replacement with UTF-16 charactertest/regression/issue/14976/14976.test.ts- template literal raw property with unicode (unskipped)test/bundler/bundler_string.test.ts- added tagged template and regex source teststest/bundler/transpiler/transpiler.test.js- added tagged template literal testtest/js/bun/shell/bunshell.test.ts- escape unicode test now expects correct output🤖 Generated with Claude Code