Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

fix(rome_js_formatter): comments in closing JSX elements #4244

Merged
merged 1 commit into from
Mar 4, 2023
Merged

fix(rome_js_formatter): comments in closing JSX elements #4244

merged 1 commit into from
Mar 4, 2023

Conversation

Conaclos
Copy link
Contributor

@Conaclos Conaclos commented Feb 26, 2023

Summary

This is a tentative to fix #4243.
This is blocking the update of prettier snapshots #4234.

I copied the code from closing_fragment.rs because the formatting is correct for fragments:

<></ /* block1 */ // test
>;

is correctly formatted to:

<></
	/* block1 */
	// test
>;

Unfortunately this has no effect:

<a></ /* block1 */ // test
a>;

still produces the following error:

$ cargo test

---- formatter::jsx_module::specs::jsx::comments_jsx stdout ----
thread 'formatter::jsx_module::specs::jsx::comments_jsx' panicked at 'formatter output had syntax errors where input had none:
comments.jsx:3:1 parse ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
  ✖ expected `<` but instead the file ends
  
    1 │ <a><//* block1 */ // test
    2 │ a>;
  > 3 │ 
      │ 
  
  ℹ the file ends here
  
    1 │ <a><//* block1 */ // test
    2 │ a>;
  > 3 │ 
      │ 

Test Plan

Test included

Documentation

No change

  • The PR requires documentation
  • I will create a new PR to update the documentation

@netlify
Copy link

netlify bot commented Feb 26, 2023

Deploy Preview for docs-rometools canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit 05510cc
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/6402456c93be470008188e2f

@Conaclos Conaclos marked this pull request as draft February 26, 2023 18:47
@Conaclos Conaclos added the Help wanted Help would be really appreciated label Feb 26, 2023
@Conaclos Conaclos changed the title fix: mis-formatting of comments in closing JSX element fix(rome_js_formatter): comments in closing JSX elements Feb 26, 2023
@ematipico ematipico added the A-Formatter Area: formatter label Feb 27, 2023
@ematipico
Copy link
Contributor

cc @MichaReiser that worked on source maps and comments

@MichaReiser
Copy link
Contributor

I recommend reading prettier's source code to understand their formatting (and how it may have changed)

@Conaclos
Copy link
Contributor Author

Conaclos commented Mar 2, 2023

I recommend reading prettier's source code to understand their formatting (and how it may have changed)

I already have an idea for addressing the issue.

My current problem is with the Rome code base. I cannot understand why my piece of code fails.
I certainly have some misunderstanding about comment handling in the formatter.

In the following code, /* comment */ is a trailing comment of /.

<a></ /* comment */a>

I think so that it was also a dangling comment of the entire JsxClosingElement </ /* comment */a>.
However, this seems not be the case?
Is there a way to collect all comments inside the node?

@MichaReiser
Copy link
Contributor

MichaReiser commented Mar 3, 2023

In the following code, /* comment */ is a trailing comment of /.

/* comment */ is a leading comment of the a node in this example because it is right before the node a starts. The comments module explains the comments handling in detail.

Our JavaScript formatter also uses an extensive list of custom-rules to place comments (same as prettier).

The way I like to approach comments related issues is to log the comments inside the test.

let syntax = SourceType::jsx();
let tree = parse("<a></ /* block1 */ // test\na>;", syntax);

let formatted = format_node(JsFormatOptions::new(syntax), &tree.syntax()).unwrap();

dbg!(&formatted.context().comments());

That helps me understand if the comments are associated with the correct nodes. You may want to compare the output with Prettier using their playground.

image

@Conaclos
Copy link
Contributor Author

Conaclos commented Mar 3, 2023

@MichaReiser
Thanks for the tips :)
I will take another look.

/* comment */ is a leading comment of the a node in this example because it is right before the node a starts.

In the rome playground, /* comment */ is a trailing trivia of /...
This could be a bug?

EDIT: I see: it is a trailing comment of / in the CST. However, the formatter made it a leading comment of the node a.

@Conaclos Conaclos marked this pull request as ready for review March 3, 2023 18:09
@Conaclos
Copy link
Contributor Author

Conaclos commented Mar 3, 2023

The fix is not ideal. However, this removes one of the issues that block the update of prettier snapshots.

Copy link
Contributor

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

How does this approach compare to prettier's handling? What do you consider non-ideal with this solution?

crates/rome_js_formatter/src/jsx/tag/closing_element.rs Outdated Show resolved Hide resolved
@Conaclos
Copy link
Contributor Author

Conaclos commented Mar 3, 2023

How does this approach compare to prettier's handling? What do you consider non-ideal with this solution?

In prettier the following code:

<a></ /* block1 */ // test
a>;

is formatted to:

<a></
  /* block1 */ // test
  a
>;

While the current fix outputs:

<a></
/* block1 */ // test
  a
>;

EDIT: it is now fixed :)

@Conaclos
Copy link
Contributor Author

Conaclos commented Mar 3, 2023

@MichaReiser I managed to match prettier output :)

It was harder than I thought to understand the formatter logic.

@ematipico
Copy link
Contributor

Amazing work @Conaclos !!!

It was harder than I thought to understand the formatter logic.

Yeah dealing with the formatter is not a easy task!

@ematipico ematipico merged commit e516c31 into rome:main Mar 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Formatter Area: formatter Help wanted Help would be really appreciated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 JsClosingElement erroneous formatting with comments
3 participants