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): port JsxChildList formatting to HtmlElementList #3782

Conversation

dyc3
Copy link
Contributor

@dyc3 dyc3 commented Sep 4, 2024

Summary

Given the similarity of JSX and HTML, I figured it would save a lot of time to simply port over the applicable portions of JSX formatting to HTML.

This PR attempts to do so for JsxChildList -> HtmlElementList.

Test Plan

quick_test should run without panicking.

Copy link

codspeed-hq bot commented Sep 4, 2024

CodSpeed Performance Report

Merging #3782 will degrade performances by 17.4%

Comparing 09-04-feat_format_html_port_jsxchildlist_formatting_to_htmlelementlist_ (dacc167) with main (eff4295)

Summary

❌ 1 regressions
✅ 106 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main 09-04-feat_format_html_port_jsxchildlist_formatting_to_htmlelementlist_ Change
eucjp_1600564308684076393.json[uncached] 953.4 µs 1,154.2 µs -17.4%

@dyc3 dyc3 added the L-HTML Language: HTML label Sep 5, 2024
@dyc3 dyc3 force-pushed the 09-04-feat_format_html_port_jsxchildlist_formatting_to_htmlelementlist_ branch from 4d8aefe to 215eecd Compare September 6, 2024 11:51
@github-actions github-actions bot added A-Project Area: project A-Tooling Area: internal tools labels Sep 6, 2024
Copy link
Contributor

github-actions bot commented Sep 6, 2024

Parser conformance results on

js/262

Test result main count This PR count Difference
Total 48511 48511 0
Passed 47320 47320 0
Failed 1191 1191 0
Panics 0 0 0
Coverage 97.54% 97.54% 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 6569 6569 0
Passed 2202 2202 0
Failed 4367 4367 0
Panics 0 0 0
Coverage 33.52% 33.52% 0.00%

ts/babel

Test result main count This PR count Difference
Total 671 671 0
Passed 599 599 0
Failed 72 72 0
Panics 0 0 0
Coverage 89.27% 89.27% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 18418 18418 0
Passed 14100 14100 0
Failed 4318 4318 0
Panics 0 0 0
Coverage 76.56% 76.56% 0.00%

@dyc3 dyc3 force-pushed the 09-04-feat_format_html_port_jsxchildlist_formatting_to_htmlelementlist_ branch from 215eecd to 62af8c2 Compare September 10, 2024 09:58
@github-actions github-actions bot removed A-Project Area: project A-Tooling Area: internal tools labels Sep 10, 2024
@dyc3 dyc3 marked this pull request as ready for review September 10, 2024 10:02
@dyc3 dyc3 requested review from a team September 10, 2024 10:02
@dyc3 dyc3 force-pushed the 09-04-feat_format_html_port_jsxchildlist_formatting_to_htmlelementlist_ branch 2 times, most recently from 61cc7b0 to 369d145 Compare September 10, 2024 14:05
@dyc3 dyc3 marked this pull request as draft September 10, 2024 14:32
@dyc3 dyc3 force-pushed the 09-04-feat_format_html_port_jsxchildlist_formatting_to_htmlelementlist_ branch from 369d145 to e80e82c Compare September 10, 2024 14:38
@dyc3 dyc3 marked this pull request as ready for review September 10, 2024 14:48
@dyc3 dyc3 marked this pull request as draft September 10, 2024 14:48
@dyc3 dyc3 force-pushed the 09-04-feat_format_html_port_jsxchildlist_formatting_to_htmlelementlist_ branch from e80e82c to edfb351 Compare September 10, 2024 15:18
@dyc3 dyc3 added the A-Formatter Area: formatter label Sep 10, 2024
@dyc3 dyc3 marked this pull request as ready for review September 10, 2024 18:29
Comment on lines +36 to +38
} else if c == '\n' {
has_newline = true;
}
Copy link
Member

Choose a reason for hiding this comment

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

Is it useful to add a comment explaining the newline detection?

Suggested change
} else if c == '\n' {
has_newline = true;
}
} else if c == '\n' {
// A newline is detected. This is tracked separately,
// potentially to differentiate text with just line breaks from other whitespace.
has_newline = true;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is explained in the doc comment for the function.

AnyHtmlElement::HtmlContent(text) => {
// Split the text into words
// Keep track if there's any leading/trailing empty line, new line or whitespace

Copy link
Member

Choose a reason for hiding this comment

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

Is this space intentional?

Copy link
Contributor Author

@dyc3 dyc3 Sep 11, 2024

Choose a reason for hiding this comment

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

This is mostly copied straight from the formatter for JsxChildList.

@dyc3

This comment was marked as resolved.

@dyc3 dyc3 marked this pull request as draft September 12, 2024 13:49
@dyc3 dyc3 force-pushed the 09-04-feat_format_html_port_jsxchildlist_formatting_to_htmlelementlist_ branch from edfb351 to 044471b Compare September 16, 2024 23:11
@dyc3 dyc3 marked this pull request as ready for review September 16, 2024 23:11
@github-actions github-actions bot added the A-Core Area: core label Sep 16, 2024
@dyc3
Copy link
Contributor Author

dyc3 commented Sep 16, 2024

I think I've resolved the comments thing in other PRs. I've tested this with the prettier test suite, and it gets most of the way there in terms of compatibility.

crates/biome_html_formatter/src/comments.rs Outdated Show resolved Hide resolved
crates/biome_html_formatter/src/html/lists/element_list.rs Outdated Show resolved Hide resolved
@dyc3 dyc3 force-pushed the 09-04-feat_format_html_port_jsxchildlist_formatting_to_htmlelementlist_ branch from 044471b to dacc167 Compare September 17, 2024 12:29
@github-actions github-actions bot added the L-JavaScript Language: JavaScript and super languages label Sep 17, 2024
@dyc3 dyc3 merged commit 1fe6f78 into main Sep 17, 2024
14 of 15 checks passed
@dyc3 dyc3 deleted the 09-04-feat_format_html_port_jsxchildlist_formatting_to_htmlelementlist_ branch September 17, 2024 13:26
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 L-HTML Language: HTML L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants