Skip to content

feat: replace external syntax error position manually#223

Merged
g-plane merged 2 commits into
mainfrom
optimize-external-fmt
May 15, 2026
Merged

feat: replace external syntax error position manually#223
g-plane merged 2 commits into
mainfrom
optimize-external-fmt

Conversation

@g-plane

@g-plane g-plane commented May 12, 2026

Copy link
Copy Markdown
Owner

Advantages

  • No need to construct large strings.
  • Syntax error position will still be mapped to original position in HTML or template.

Disadvantages

  • API changed: Error type from external formatter must be anyhow::Error instead of generic E, but this should be okay because most users will consume the built wasm instead of calling from Rust. Even though, most errors type can be converted to anyhow::Error.
  • Error message recognition rule are encoded in a regex, and it may not be able to handle any external formatters. Of course, plugins listed in dprint.dev are supported.

Result

Given the code below:

<script lang="ts">


     {
</script>

<style>

    {
</style>

<script type="application/json">

   {
</script>

errors will be like:

Expected '}', got '<eof>' at file:///file.tsExpected '}', got '<eof>' at file:///file.ts:4:6

       {
       ~

       {
       ~
syntax error at syntax error at line 9, col 5: CSS rule is expected: CSS rule is expected
Line 14, column 4: Unterminated object

     {: Unterminated object

     {

Close #221

@UnknownPlatypus @sergey-kintsel

@netlify

netlify Bot commented May 12, 2026

Copy link
Copy Markdown

Deploy Preview for markup-fmt ready!

Name Link
🔨 Latest commit 94f616c
🔍 Latest deploy log https://app.netlify.com/projects/markup-fmt/deploys/6a03b9aa396fb90008db27c6
😎 Deploy Preview https://deploy-preview-223--markup-fmt.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@sergey-kintsel sergey-kintsel left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I ran the new version with the 10MB html file I mentioned before. it worked quite quickly - 1.9 seconds in total, didn't panic.
could we fix the output though? because right now the error message seems to be duplicating itself

syntax error at syntax error at line 199175, col 55: expect token `{`, but found `;`: expect token `{`, but found `;`

Comment thread markup_fmt/src/ctx.rs Outdated
Comment thread markup_fmt/src/ctx.rs
@g-plane g-plane merged commit e88e271 into main May 15, 2026
5 checks passed
@g-plane g-plane deleted the optimize-external-fmt branch May 15, 2026 03:52
Xavrir added a commit to Xavrir/deno that referenced this pull request Jun 1, 2026
`deno fmt` did not terminate on large HTML files containing many inline
`style="..."` attributes. markup_fmt 0.27.0's `format_style_attr` prepended a
whitespace-masked copy of the entire preceding document to every inline style
before handing it to the CSS formatter, making formatting O(n^2) in the number
of style attributes.

markup_fmt 0.27.2 (g-plane/markup_fmt#223) removed that prefix and instead
computes source line/column lazily only on error, restoring linear time.
Bumping the pin picks up the fix.

Closes denoland#30982
Xavrir added a commit to Xavrir/deno that referenced this pull request Jun 1, 2026
`deno fmt` did not terminate on large HTML files containing many inline
`style="..."` attributes. markup_fmt 0.27.0's `format_style_attr` prepended a
whitespace-masked copy of the entire preceding document to every inline style
before handing it to the CSS formatter, making formatting O(n^2) in the number
of style attributes.

markup_fmt 0.27.2 (g-plane/markup_fmt#223) removed that prefix and instead
computes source line/column lazily only on error, restoring linear time.
Bumping the pin picks up the fix.

Adds a fmt spec test that formats an HTML file with many inline style
attributes and asserts it completes.

Closes denoland#30982
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.

2 participants