Skip to content

fix: indent content added with append/prepend - #343

Open
theRizwan wants to merge 1 commit into
Rich-Harris:masterfrom
theRizwan:fix-indent-inserted-content
Open

fix: indent content added with append/prepend#343
theRizwan wants to merge 1 commit into
Rich-Harris:masterfrom
theRizwan:fix-indent-inserted-content

Conversation

@theRizwan

Copy link
Copy Markdown
Contributor

indent() walks the original characters and the edited chunk contents, but it never
looks at the intro/outro a chunk picks up from appendLeft, appendRight,
prependLeft and prependRight. So inserted content is invisible to the walk, and
two things go wrong:

  1. a line that starts inside inserted content is never prefixed;
  2. a line break inside inserted content does not start a new line as far as the walk
    is concerned, so the original code that follows the insert silently loses its
    indent.

(2) is the damaging one — it mis-indents code the caller never touched.

Reproductions

All checked against the published magic-string@1.2.3 build.

const s = new MagicString('var a = 1;\nvar b = 2;')

s.prepend('(function () {\n')
s.prependRight(11, 'debugger;\n')
s.append('\n}());')

s.toString()
// '(function () {\nvar a = 1;\ndebugger;\nvar b = 2;\n}());'

s.indent('  ')
s.toString()
// actual   '  (function () {\n  var a = 1;\n  debugger;\nvar b = 2;\n}());'
// expected '  (function () {\n  var a = 1;\n  debugger;\n  var b = 2;\n  }());'

Wrapping a module and indenting it is the canonical use for indent(), and here the
last two lines come out flush left.

The individual failures, all on 'a\nb\nc' with indent('>'):

ops toString() indent('>') expected
prependRight(2, 'Q\n') a\nQ\nb\nc >a\n>Q\n**b**\n>c >a\n>Q\n>b\n>c
appendLeft(1, '\nQ') a\nQ\nb\nc >a\n**Q**\n>b\n>c >a\n>Q\n>b\n>c
appendLeft(2, 'X') a\nXb\nc >a\n**X>b**\n>c >a\n>Xb\n>c
append('\nZ') a\nb\nc\nZ >a\n>b\n>c\n**Z** >a\n>b\n>c\n>Z

The last one does not even involve a chunk: this.outro is run through the pattern,
but the replacer's "am I continuing a line?" guard is applied to every match rather
than only the one at offset 0, and a match further into the string always follows a
line break. Bundle#indent already gets this right for its own intro
(index > 0 ? indentStr + match : match).

The fix

Indent the pieces in output order — chunk intro, then the content, then chunk
outro — with the same helper used for the string-level intro/outro, so the
line-start tracking stays accurate across inserted content. The helper also fixes the
offset-0 guard, and subsumes the ad-hoc flag update the edited-chunk branch was doing.

One knock-on: when a line starts at a chunk boundary the indent is now appendRighted
onto the intro rather than prependRighted, so it lands directly in front of the
content instead of in front of an intro that has already been indented in its own
right. That is what turns X>b into >Xb in the third row above.

Exclusion ranges are honoured for inserted content too: a chunk's intro is skipped
when chunk.start is excluded, its outro when chunk.end - 1 is excluded, matching
how the existing branches treat excluded original characters.

Evidence

Differential harness: random op sequences (append/prepend/appendLeft/
appendRight/prependLeft/prependRight/remove/overwrite) over seven originals,
comparing indent(prefix) against prefixing every line of that same MagicString's own
toString().

  • master: 10,920 of 19,410 cases disagree
  • this branch: 0 of 19,410

A separate sweep generated hires sourcemaps for 3,603 indented cases and found no
out-of-bounds mapping segments.

262 existing tests still pass with no changed expectations — nothing in the suite
pinned the old behaviour. 9 tests added (271 total); 7 of them fail on master, the
other 2 are guards for behaviour that is deliberately preserved (content that
continues a line is not prefixed, and indentStart: false still suppresses the first
indent even when the string opens with an insert).

Lint and typecheck clean.

Not addressed

A lone \r left behind by splitting a \r\n pair is not treated as a line terminator,
so nothing is indented after it. That is pre-existing and orthogonal — indent() only
ever recognised \n — so I have left it alone rather than widen this PR.

`indent()` walked the original characters and the edited chunk contents, but
never looked at the `intro`/`outro` a chunk picks up from `appendLeft`,
`appendRight`, `prependLeft` and `prependRight`. Two things went wrong:

- a line that starts inside inserted content was never prefixed
- a line break inside inserted content did not start a new line as far as the
  walk was concerned, so the *original* code after the insert lost its indent

Inserted content is now indented in output order — chunk `intro`, then the
content, then chunk `outro` — using the same helper as the string-level
`intro`/`outro`, which also keeps the line-start tracking accurate across it.

Two smaller fixes fall out of that:

- the replacer only skipped the indent when it was continuing a line, but it
  applied that to every match, not just the one at offset 0; a match further
  in always follows a line break, so `s.append('\nZ')` left `Z` unindented
- when a line starts at a chunk boundary the indent is appended to the intro
  rather than prepended, so it lands in front of the content instead of in
  front of an intro that has already been indented on its own
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant