feat: nativeMagicString lastChar#7819
Merged
graphite-app[bot] merged 1 commit intomainfrom Jan 9, 2026
Merged
Conversation
This was referenced Jan 9, 2026
Member
Author
How to use the Graphite Merge QueueAdd the label graphite: merge-when-ready to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
IWANABETHATGUY
force-pushed
the
01-09-feat_nativemagicstring_lastchar
branch
from
aa3e637 to
12964ca
Compare
IWANABETHATGUY
force-pushed
the
01-09-feat_nativemagicstring_lastchar
branch
from
12964ca to
fae065d
Compare
hyf0
approved these changes
Jan 9, 2026
graphite-app
bot
changed the base branch from
01-09-feat_nativemagicstring_snip
to
graphite-base/7819
graphite-app
bot
force-pushed
the
graphite-base/7819
branch
from
0975145 to
f4f0f5b
Compare
graphite-app
bot
force-pushed
the
01-09-feat_nativemagicstring_lastchar
branch
from
fae065d to
b148ce9
Compare
graphite-app
bot
force-pushed
the
01-09-feat_nativemagicstring_lastchar
branch
from
b148ce9 to
03d234f
Compare
✅ Deploy Preview for rolldown-rs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
Merge activity
|
graphite-app
bot
force-pushed
the
01-09-feat_nativemagicstring_lastchar
branch
from
03d234f to
5befdf0
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements lastChar() and lastLine() methods for the native MagicString implementation, bringing the binding closer to feature parity with the original magic-string library.
Key changes:
- Adds Rust implementations of
last_char()andlast_line()methods in the string_wizard crate - Exposes these methods through the NAPI binding layer
- Updates TypeScript type definitions with proper documentation
- Enables the
lastLinetest suite that was previously skipped
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/string_wizard/src/magic_string/mod.rs | Implements core last_char() and last_line() methods with logic to traverse chunks and VecDeques |
| crates/rolldown_binding/src/types/binding_magic_string.rs | Adds NAPI bindings for last_char() and last_line() with appropriate type conversions |
| packages/rolldown/src/binding.d.cts | Adds TypeScript type definitions and documentation for the new methods |
| packages/rolldown/tests/magic-string/download-tests.mjs | Updates documentation to reflect that lastChar and lastLine are now supported |
| packages/rolldown/tests/magic-string/MagicString.test.ts | Enables the lastLine test suite |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }); | ||
|
|
||
| describe.skip('lastLine', () => { | ||
| describe('lastLine', () => { |
There was a problem hiding this comment.
The lastLine test block was enabled but there's no corresponding test block for lastChar. Since lastChar is listed as a newly supported method (line 37 in download-tests.mjs), it should have test coverage. Either add a test block for lastChar or ensure that the upstream magic-string test suite includes these tests.
Comment on lines
+118
to
+122
| if let Some(last_outro) = chunk.outro.back() { | ||
| if let Some(c) = last_outro.chars().last() { | ||
| return Some(c); | ||
| } | ||
| } |
There was a problem hiding this comment.
This logic has a bug when VecDeque elements are empty strings. If the last element of outro/intro VecDeques is an empty string, the code skips past all other elements in that VecDeque. For example, if chunk.outro is [" ", ""], this code will check only "" (which returns None) and move on, missing the ' ' character in the first element. The code should iterate through all elements in reverse order using iter().rev() and check each for the last character, similar to how last_line handles this on lines 171-175.
Comment on lines
+135
to
+139
| if let Some(last_intro) = chunk.intro.back() { | ||
| if let Some(c) = last_intro.chars().last() { | ||
| return Some(c); | ||
| } | ||
| } |
There was a problem hiding this comment.
Same bug as with chunk.outro - if the last element of chunk.intro is empty, this code won't check earlier elements in the VecDeque. This should iterate through intro elements in reverse order to handle empty strings correctly.
Comment on lines
+145
to
+149
| if let Some(last_intro) = self.intro.back() { | ||
| if let Some(c) = last_intro.chars().last() { | ||
| return Some(c); | ||
| } | ||
| } |
There was a problem hiding this comment.
Same bug as with the chunk intro/outro - if the last element of the global intro VecDeque is empty, earlier elements won't be checked. This should iterate through intro elements in reverse order.
Comment on lines
+106
to
+110
| if let Some(last_outro) = self.outro.back() { | ||
| if let Some(c) = last_outro.chars().last() { | ||
| return Some(c); | ||
| } | ||
| } |
There was a problem hiding this comment.
Same bug as with intro - if the last element of the global outro VecDeque is empty, earlier elements won't be checked. This should iterate through outro elements in reverse order.
Comment on lines
+155
to
+211
| pub fn last_line(&self) -> String { | ||
| // Check outro first (last in output order) | ||
| for outro_part in self.outro.iter().rev() { | ||
| if let Some(line_index) = memchr::memrchr(b'\n', outro_part.as_bytes()) { | ||
| return outro_part[line_index + 1..].to_string(); | ||
| } | ||
| } | ||
|
|
||
| let mut line_str = self.outro.iter().map(|s| s.as_ref()).collect::<String>(); | ||
|
|
||
| // Traverse chunks from last to first | ||
| let mut chunk_idx = Some(self.last_chunk_idx); | ||
| while let Some(idx) = chunk_idx { | ||
| let chunk = &self.chunks[idx]; | ||
|
|
||
| // Check chunk outro | ||
| for outro_part in chunk.outro.iter().rev() { | ||
| if let Some(line_index) = memchr::memrchr(b'\n', outro_part.as_bytes()) { | ||
| return outro_part[line_index + 1..].to_string() + &line_str; | ||
| } | ||
| } | ||
| let chunk_outro: String = chunk.outro.iter().map(|s| s.as_ref()).collect(); | ||
| line_str = chunk_outro + &line_str; | ||
|
|
||
| // Check chunk content (edited or original) | ||
| let content = chunk | ||
| .edited_content | ||
| .as_ref() | ||
| .map(|s| s.as_ref()) | ||
| .unwrap_or_else(|| chunk.span.text(&self.source)); | ||
| if let Some(line_index) = memchr::memrchr(b'\n', content.as_bytes()) { | ||
| return content[line_index + 1..].to_string() + &line_str; | ||
| } | ||
| line_str = content.to_string() + &line_str; | ||
|
|
||
| // Check chunk intro | ||
| for intro_part in chunk.intro.iter().rev() { | ||
| if let Some(line_index) = memchr::memrchr(b'\n', intro_part.as_bytes()) { | ||
| return intro_part[line_index + 1..].to_string() + &line_str; | ||
| } | ||
| } | ||
| let chunk_intro: String = chunk.intro.iter().map(|s| s.as_ref()).collect(); | ||
| line_str = chunk_intro + &line_str; | ||
|
|
||
| chunk_idx = chunk.prev; | ||
| } | ||
|
|
||
| // Check intro last (first in output order, but we're going backwards) | ||
| for intro_part in self.intro.iter().rev() { | ||
| if let Some(line_index) = memchr::memrchr(b'\n', intro_part.as_bytes()) { | ||
| return intro_part[line_index + 1..].to_string() + &line_str; | ||
| } | ||
| } | ||
|
|
||
| let intro_str: String = self.intro.iter().map(|s| s.as_ref()).collect(); | ||
| intro_str + &line_str | ||
| } |
There was a problem hiding this comment.
This implementation has performance issues due to multiple string allocations. Each iteration creates new strings through concatenation (lines 177, 188, 197), and the final result at line 210 creates another allocation. For large strings or many chunks, this could be inefficient. Consider using a single String buffer and writing to it in reverse order, or collecting all parts into a Vec and joining them once at the end.
Contributor
Benchmarks Rust |
This was referenced Jan 14, 2026
shulaoda
added a commit
that referenced
this pull request
Jan 14, 2026
## [1.0.0-beta.60] - 2026-01-14 ### 💥 BREAKING CHANGES - tsconfig: enable auto-discovery by default (#7817) by @shulaoda ### 🚀 Features - distinguish transformer diagnostics from parse errors (#7872) by @shulaoda - emit transformer warnings instead of ignoring them (#7850) by @shulaoda - node: add `output.codeSplitting` option and deprecate `output.advancedChunks` (#7855) by @hyf0 - nativeMagicString reset (#7828) by @IWANABETHATGUY - nativeMagicString lastChar (#7819) by @IWANABETHATGUY - dev/lazy: inject lazy compilation runtime automatically (#7816) by @hyf0 - nativeMagicString snip (#7818) by @IWANABETHATGUY - nativeMagicString construct with options (#7814) by @IWANABETHATGUY - nativeMagicString clone (#7813) by @IWANABETHATGUY - nativeMagicString `insert` (#7812) by @IWANABETHATGUY - nativeMagicString `slice` (#7807) by @IWANABETHATGUY - nativeMagicString trim methods (#7800) by @IWANABETHATGUY - make closeBundle hook receive the last error (#7278) by @Copilot ### 🐛 Bug Fixes - when package only contains export default, cjsDefault didn't resolve correctly (#7873) by @IWANABETHATGUY - inline __name calls for default exports (#7862) by @IWANABETHATGUY - improve variable renaming to avoid unnecessary shadowing in nested scopes (#7859) by @IWANABETHATGUY - use correct index when inserting keepNames statements during export default transformation (#7853) by @IWANABETHATGUY - transform non-static dynamic imports when `dynamicImportInCjs` is `false` (#7823) by @shulaoda - dev/lazy: should include imported and non-executed modules in the patch (#7815) by @hyf0 - set ExportsKind to Esm when json is none object literal (#7808) by @IWANABETHATGUY - nativeMagicString move api (#7796) by @IWANABETHATGUY - remove unnecessary exports after merging into commong and user defined entry (#7789) by @IWANABETHATGUY - use output.name instead of chunk.name in mixed export warning (#7788) by @Copilot ### 🚜 Refactor - generalize ParseError to OxcError with dynamic EventKind (#7868) by @shulaoda - rust: rename `advanced_chunks` to `manual_code_splitting` (#7856) by @hyf0 - string_wizard error hanlding (#7830) by @IWANABETHATGUY - remove `experimental.disableLiveBindings` option (#7820) by @sapphi-red - node/test: run fixture tests in concurrent (#7790) by @hyf0 - move ConfigExport and RolldownOptionsFunction types to define-config (#7799) by @shulaoda - cli: validate config after resolving and improve error message (#7798) by @shulaoda ### 📚 Documentation - rebrand (#7670) by @yyx990803 - fix incorrect default value for propertyReadSideEffects (#7847) by @Copilot - remove options pages and redirect to reference pages (#7834) by @sapphi-red - options: inline types to option property pages (#7831) by @sapphi-red - options: port checks.pluginTimings content from options page to reference page (#7832) by @sapphi-red - options: use `@linkcode` where possible (#7824) by @sapphi-red - options: port content from options page to reference page (#7822) by @sapphi-red - options: add descriptions for output options (#7821) by @sapphi-red - options: add description for input options (#7802) by @sapphi-red - options: add description for `checks.*` (#7801) by @sapphi-red - apis: add hook graph (#7671) by @sapphi-red ### 🧪 Testing - add all valid combination of chunk exports related test (#7851) by @IWANABETHATGUY - enable MagicString test after api return type alignment (#7797) by @IWANABETHATGUY - init magic-string test (#7794) by @IWANABETHATGUY ### ⚙️ Miscellaneous Tasks - vite-tests: configure git user for rebase operation (#7875) by @shulaoda - rolldown_binding: remove v3 native plugins (#7837) by @shulaoda - rolldown_binding: allow crate-type as lib (#7866) by @Brooooooklyn - README.md: adjust position and size of rolldown logo (#7861) by @hyf0 - deps: update test262 submodule for tests (#7857) by @sapphi-red - deps: update oxc to v0.108.0 (#7845) by @renovate[bot] - deps: update dependency oxlint to v1.39.0 (#7849) by @renovate[bot] - deps: update dependency oxfmt to ^0.24.0 (#7844) by @renovate[bot] - deps: update npm packages (#7841) by @renovate[bot] - deps: update rust crates (#7839) by @renovate[bot] - deps: update github-actions (#7840) by @renovate[bot] - use workspace edition for all crates (#7829) by @IWANABETHATGUY - deps: update dependency oxlint-tsgolint to v0.11.0 (#7827) by @renovate[bot] - deps: update napi to v3.8.2 (#7810) by @renovate[bot] - remove outdated snapshot files (#7806) by @shulaoda - deps: update crate-ci/typos action to v1.42.0 (#7792) by @renovate[bot] Co-authored-by: shulaoda <[email protected]>
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
2 participants
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.
No description provided.