feature: Added formatting for self-closing JSX element #2273

NicholasLYang · 2022-03-21T20:52:32Z

Summary

Formats JSX self-closing elements (#2272). Doesn't implement formatting for attributes or element names.

Test Plan

Added test file self_closing.jsx with some examples, some of which are pulled from rome_playground

cloudflare-workers-and-pages · 2022-03-21T20:58:38Z

Deploying with Cloudflare Pages

Latest commit:	`c3e4e31`
Status:	✅ Deploy successful!
Preview URL:	https://93509e6a.tools-8rn.pages.dev

View logs

MichaReiser · 2022-03-22T14:23:05Z

Nit: Would you mind filling out the PR template. You can go into some more detail. Like does it support formatting props too? Or just empty self closing elements? How did you test it? Did you add new test cases?

NicholasLYang · 2022-03-22T14:33:52Z

Do we want a proper folder structure for JSX tests? I can have it mirror the JSX formatter directory structure

ematipico

The snapshot shows that there are a lot of unimplemented nodes (they still format verbatim).

ematipico · 2022-03-22T14:30:02Z

crates/rome_formatter/tests/specs/jsx/self_closing.jsx.snap

+/>;
+
+
+## Unimplemented nodes/tokens


If the snapshot has this section, it means that we are not formatting some tokens/nodes. This means that a test should be valid if it doesn't have this section.

Yeah this was on purpose because I didn't want to go down the rabbit hole of attribute/name formatting. If this is necessary I can put this PR on hold and implement all of that.

I think going step by step is fine.

Exactly, we don't need to format everything in one go. Let's just focus on the small feature and gradually add things along the way. Simply formatting <Foo /> is just fine

ematipico · 2022-03-22T14:31:11Z

crates/rome_formatter/tests/specs/jsx/self_closing.jsx

+<LineWidthInput lineWidth={lineWidth} setLineWidth={setLineWidth} />;
+<IndentStyleSelect indentWidth={indentWidth} setIndentWidth={setIndentWidth} indentStyle={indentStyle} setIndentStyle={setIndentStyle}
+/>;
+<SourceTypeSelect
+	isTypeScript={isTypeScript}
+
+	setIsTypeScript={setIsTypeScript}
+
+	isJsx={isJsx}
+
+	setIsJsx={setIsJsx}
+/>;
+
+<CodeEditor
+	value={code}
+	language="js"
+	placeholder="Enter JS here"
+	onChange={(evn) => {
+		setCode(evn.target.value);
+	}}
+	style={{ fontSize: 12, height: "100vh", fontFamily:
+			"ui-monospace,SFMono-Regular,SF Mono,Consolas,Liberation Mono,Menlo,monospace",
+	}}
+/>


Since the PR should be focusing only on self closing tags, I would suggest to remove everything around attributes, which are out of scope.

Well the formatting does include how we place attributes, so it is important that we have tests with attributes. Should I remove the non trivial ones then?

Sure, but the PR is exclusively about self closing tags, and having formatting for attributes can be done with a different PR without dependencies, that's why I created a task just for that.

Hmm okay. I was mostly testing this code namely the soft line breaks around attributes.

We can iterate again over the line breaks when we will implement the format of attributes.

You can leave the implementation as it is, the most important part is that the test doesn't have to have JSXAttributes.

ematipico · 2022-03-22T14:33:26Z

crates/rome_formatter/tests/specs/jsx/self_closing.jsx.snap

+"Foo " => 1..5
+"Foo           " => 11..25


Not sure if you noticed, but Foo is not actually formatted, (it formatted via format_verbatim), which means it's not correct.

Probably we have to implement JsxText node.

Hmm. I'm confused here because it does correctly format <Foo />; as <Foo />. Perhaps it's just printing the name part (which is purposefully not formatted) with trivia?

The reason why the second Foo have the spaces removed, is because we actually trim the tokens out from the verbatim tokens (the first leading and the first trailing), and by chance the first trailing trivia is a white space. You could try to insert some new lines and could see the different.

Anyway, if you check the AST:

tag: JsxSelfClosingElement { l_angle_token: [email protected] "<" [] [], name: JsxReferenceIdentifier { value_token: [email protected] "Foo" [] [Whitespace(" ")], }, type_arguments: missing (optional), attributes: JsxAttributeList [], slash_token: [email protected] "/" [] [], r_angle_token: [email protected] ">" [] [], },

name is a JsxReferenceIdentifier which is still formatting verbatim

I don't handle any formatting of JsxAnyElementName. Should I?

Yeah you really should, at least to cover the small test of your PR

ematipico · 2022-03-22T14:35:07Z

You can check this section: https://github.com/rome/tools/blob/main/crates/rome_formatter/docs/implement_the_formatter.md to make sure to understand when a test is correct.

MichaReiser · 2022-03-24T10:19:18Z

crates/rome_formatter/src/jsx/tag/self_closing_element.rs

+        for attribute in self.attributes() {
+            attributes.push(attribute.format(formatter)?);
+            attributes.push(soft_line_break_or_space());
+        }


You can use join_elements for this together with format_nodes

Suggested change

for attribute in self.attributes() {

attributes.push(attribute.format(formatter)?);

attributes.push(soft_line_break_or_space());

}

let attributes = join_elements(soft_line_break_or_space(), formatter.format_nodes(self.attributes());

Hmm so I switched to this with a tweak:

let mut attributes = Vec::new(); for attribute in self.attributes() { attributes.push(attribute.format(formatter)?); attributes.push(soft_line_break_or_space()); }

But then for some reason the lines always became a hard line break. I don't really know why.

If possible, we should avoid creating new vectors. join_elements uses concat_elements under the hoods, which is really optimized around the usage of new vectors.

github-actions · 2022-04-05T16:41:17Z

Parser conformance results on ubuntu-latest

js/262

Test result	`main` count	This PR count	Difference
Total	45250	45250	0
Passed	44310	44310	0
Failed	940	940	0
Panics	0	0	0
Coverage	97.92%	97.92%	0.00%

jsx/babel

Test result	`main` count	This PR count	Difference
Total	39	39	0
Passed	36	36	0
Failed	3	3	0
Panics	0	0	0
Coverage	92.31%	92.31%	0.00%

ts/babel

Test result	`main` count	This PR count	Difference
Total	584	584	0
Passed	508	508	0
Failed	76	76	0
Panics	0	0	0
Coverage	86.99%	86.99%	0.00%

ts/microsoft

Test result	`main` count	This PR count	Difference
Total	15983	15983	0
Passed	12167	12167	0
Failed	3816	3816	0
Panics	0	0	0
Coverage	76.12%	76.12%	0.00%

crates/rome_js_formatter/src/jsx/tag/self_closing_element.rs

ematipico · 2022-04-11T11:56:31Z

crates/rome_js_formatter/tests/specs/jsx/self_closing.jsx.snap

+"Foo " => 1..5
+"Foo           " => 11..25
+"LineWidthInput " => 31..46
+"IndentStyleSelect" => 51..68
+"SourceTypeSelect" => 75..91
+"CodeEditor" => 99..109


The presence of these information in the snapshot means that we haven't implemented these nodes: https://github.com/rome/tools/blob/main/xtask/codegen/js.ungram#L2202-L2203 (they still format verbatim)

As general rule, if we have this information, we consider the test as invalid: https://github.com/rome/tools/blob/main/crates/rome_js_formatter/docs/write_tests.md#identify-issues

some tokens haven't been printed; usually you will have this information inside the snapshot, under a section called "Unimplemeted tokens/nodes"; a test, in order to be valid, can't have that section;

I thought we had agreed that JsxName formatting was out of scope. But no worries, implemented it

MichaReiser · 2022-04-12T14:37:54Z

crates/rome_js_formatter/src/jsx/tag/self_closing_element.rs

+        let mut attributes = Vec::new();
+        for attribute in self.attributes() {
+            attributes.push(attribute.format(formatter)?);
+            attributes.push(soft_line_break_or_space());
+        }


You can use join_elements(formatter.format_nodes(self.attributes())?, soft_line_break_or_space()) to get the same result.

I had the weirdest issue with that last time where it added line breaks when it shouldn't have. No clue why, but switched to it now.

NicholasLYang marked this pull request as ready for review March 22, 2022 14:20

NicholasLYang requested review from ematipico and leops March 22, 2022 14:21

ematipico suggested changes Mar 22, 2022

View reviewed changes

NicholasLYang requested a review from ematipico March 23, 2022 17:15

MichaReiser reviewed Mar 24, 2022

View reviewed changes

NicholasLYang force-pushed the feature/jsx-self-closing-element branch from 904c0bc to b4a36c6 Compare April 5, 2022 16:38

ematipico reviewed Apr 6, 2022

View reviewed changes

crates/rome_js_formatter/src/jsx/tag/self_closing_element.rs Outdated Show resolved Hide resolved

NicholasLYang requested review from ematipico and MichaReiser April 8, 2022 16:00

ematipico suggested changes Apr 11, 2022

View reviewed changes

ematipico approved these changes Apr 11, 2022

View reviewed changes

Nicholas Yang added 8 commits April 11, 2022 16:11

feature: Added formatting for self-closing JSX element

d288a56

test: Added a test for self closing elements

8c9c8a4

fix: Removed attributes from test

d03c367

Updating prettier tests

57df30b

fix: fixing test

d26f850

feature: Added formatting for type argument in self closing element

b1e4f09

Switched to using format_or_empty

659bbd3

Added jsx name formatting

e4197d7

NicholasLYang force-pushed the feature/jsx-self-closing-element branch from 71490d7 to e4197d7 Compare April 12, 2022 14:06

fix: Test spec output

c3e4e31

MichaReiser reviewed Apr 12, 2022

View reviewed changes

fix: switched to join_elements

7eb3e9f

NicholasLYang merged commit 9f6312b into main Apr 12, 2022

NicholasLYang deleted the feature/jsx-self-closing-element branch April 12, 2022 14:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: Added formatting for self-closing JSX element #2273

feature: Added formatting for self-closing JSX element #2273

NicholasLYang commented Mar 21, 2022 •

edited

Loading

cloudflare-workers-and-pages bot commented Mar 21, 2022 •

edited

Loading

MichaReiser commented Mar 22, 2022

NicholasLYang commented Mar 22, 2022

ematipico left a comment

ematipico Mar 22, 2022

NicholasLYang Mar 22, 2022

MichaReiser Mar 22, 2022

ematipico Mar 22, 2022

ematipico Mar 22, 2022

NicholasLYang Mar 22, 2022

ematipico Mar 22, 2022

NicholasLYang Mar 22, 2022

ematipico Mar 23, 2022

ematipico Mar 22, 2022

NicholasLYang Mar 22, 2022

ematipico Mar 23, 2022 •

edited

Loading

NicholasLYang Apr 5, 2022

ematipico Apr 11, 2022

ematipico commented Mar 22, 2022

MichaReiser Mar 24, 2022

NicholasLYang Apr 5, 2022

ematipico Apr 11, 2022

github-actions bot commented Apr 5, 2022 •

edited

Loading

ematipico Apr 11, 2022 •

edited

Loading

NicholasLYang Apr 11, 2022

MichaReiser Apr 12, 2022

NicholasLYang Apr 12, 2022

		"Foo " => 1..5
		"Foo " => 11..25

feature: Added formatting for self-closing JSX element #2273

feature: Added formatting for self-closing JSX element #2273

Conversation

NicholasLYang commented Mar 21, 2022 • edited Loading

Summary

Test Plan

cloudflare-workers-and-pages bot commented Mar 21, 2022 • edited Loading

Deploying with Cloudflare Pages

MichaReiser commented Mar 22, 2022

NicholasLYang commented Mar 22, 2022

ematipico left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ematipico Mar 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ematipico commented Mar 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 5, 2022 • edited Loading

Parser conformance results on ubuntu-latest

js/262

jsx/babel

ts/babel

ts/microsoft

ematipico Apr 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicholasLYang commented Mar 21, 2022 •

edited

Loading

cloudflare-workers-and-pages bot commented Mar 21, 2022 •

edited

Loading

ematipico Mar 23, 2022 •

edited

Loading

github-actions bot commented Apr 5, 2022 •

edited

Loading

ematipico Apr 11, 2022 •

edited

Loading