Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(format/html): implement suppression comments #5356

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dyc3
Copy link
Contributor

@dyc3 dyc3 commented Mar 14, 2025

Summary

This reverts #4056 to make HTML comments actual comment trivia. It also attempts to fix the formatting for comments as well, trying to preserve whitespace in the same places that prettier does.

This PR is also the result of a cascading clusterfuck of issues that appeared as a result of making this change, so it may seem a little scatterbrained, and a little difficult to review.

One unfortunate regression is this case: crates/biome_html_formatter/tests/specs/html/comments/after-text-inside-div.html

<div>
  Foo

  <!-- Bar -->
</div>

Prettier treats comments as actual nodes in the AST, it treats the comment as a child of the div. However, because this PR makes Biome not do that, the comment gets attached as trivia to the closing tag. Ultimately, this results in unstable formatting. I originally wanted to fix that in this PR, but it's too complicated and this PR is already kinda big.

This PR also doesn't attempt to handle or reimplement prettier-ignore-attribute comments in any way. Example from prettier docs:

<!-- prettier-ignore -->
<div         class="x"       >hello world</div            >

<!-- prettier-ignore-attribute -->
<div
  (mousedown)="       onStart    (    )         "
  (mouseup)="         onEnd      (    )         "
></div>

<!-- prettier-ignore-attribute (mouseup) -->
<div
  (mousedown)="onStart()"
  (mouseup)="         onEnd      (    )         "
></div>

The reason these exist is because you can't put comments inside the <> of HTML tags.

Test Plan

CI should pass. Added new tests, and updated snapshots.

@github-actions github-actions bot added A-Parser Area: parser A-Formatter Area: formatter A-Tooling Area: internal tools L-HTML Language: HTML labels Mar 14, 2025
Copy link
Contributor

github-actions bot commented Mar 14, 2025

Parser conformance results on

js/262

Test result main count This PR count Difference
Total 50437 50437 0
Passed 49122 49122 0
Failed 1315 1315 0
Panics 0 0 0
Coverage 97.39% 97.39% 0.00%

jsx/babel

Test result main count This PR count Difference
Total 40 40 0
Passed 37 37 0
Failed 3 3 0
Panics 0 0 0
Coverage 92.50% 92.50% 0.00%

symbols/microsoft

Test result main count This PR count Difference
Total 6645 6645 0
Passed 2229 2229 0
Failed 4416 4416 0
Panics 0 0 0
Coverage 33.54% 33.54% 0.00%

ts/babel

Test result main count This PR count Difference
Total 798 798 0
Passed 706 706 0
Failed 92 92 0
Panics 0 0 0
Coverage 88.47% 88.47% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 18667 18667 0
Passed 14316 14316 0
Failed 4351 4351 0
Panics 0 0 0
Coverage 76.69% 76.69% 0.00%

Copy link

codspeed-hq bot commented Mar 14, 2025

CodSpeed Performance Report

Merging #5356 will not alter performance

Comparing html-suppression-comments (100dcd5) with main (e54b047)

Summary

✅ 95 untouched benchmarks

@dyc3 dyc3 force-pushed the html-suppression-comments branch from 9700f3a to 89290e1 Compare March 14, 2025 22:05
@dyc3 dyc3 force-pushed the html-suppression-comments branch from 89290e1 to 5c441aa Compare April 7, 2025 13:33
@github-actions github-actions bot added the A-Core Area: core label Apr 7, 2025
@dyc3 dyc3 force-pushed the html-suppression-comments branch from 5c441aa to 100dcd5 Compare April 8, 2025 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Core Area: core A-Formatter Area: formatter A-Parser Area: parser A-Tooling Area: internal tools L-HTML Language: HTML
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant