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

fix(rome_js_formatter): restrict export line break for single specifier #3726

Merged
merged 11 commits into from
Nov 16, 2022
Merged

fix(rome_js_formatter): restrict export line break for single specifier #3726

merged 11 commits into from
Nov 16, 2022

Conversation

unvalley
Copy link
Contributor

@unvalley unvalley commented Nov 14, 2022

Summary

  • a part of fix this comment for prettier compatibility
  • restrict line breaks when specifier of export is long
  • format output is below
// given code
export { loooooooooooooooooooooooooooooooooooooooooooooooooong } from "loooooooooooooooooooooooooooooooooooooooooooooong"

// formated (Before merging this PR)
export { 
  loooooooooooooooooooooooooooooooooooooooooooooooooong,
} from "loooooooooooooooooooooooooooooooooooooooooooooong"


// formated (After merging this PR ideally)
export { loooooooooooooooooooooooooooooooooooooooooooooooooong } from "loooooooooooooooooooooooooooooooooooooooooooooong"

Test Plan

  • updated js/module/export/named_from_clause.js snapshot
  • updated prettier_tests prettier/js/export-extension/export.js snapshot

@netlify
Copy link

netlify bot commented Nov 14, 2022

Deploy Preview for docs-rometools canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit c2042dc
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/6373ad87cbb2700008096180

Comment on lines 35 to 37
let first_specifier = specifiers.elements().next().unwrap();
match (first_specifier.node(), first_specifier.trailing_separator()) {
(Ok(specifier), Ok(_separator)) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've referenced import_named_clause.

// SAFETY: we know that the `specifiers.specifiers().len() == 1`, so unwrap `iter().next()` is safe.

Copy link
Contributor

@MichaReiser MichaReiser Nov 15, 2022

Choose a reason for hiding this comment

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

We should fall back to the default implementation if specifier or separator isn't ok, and we have to call format_removed if the separator is not none.

We should also check that the specifier has no comments

It can then be written like this (you have to make the node and trailing_separator fields pub on AstSeparatedElement

match specifiers.elements().next() {
    Some(AstSeparatedElement {
        node: Ok(node),
        trailing_separator: Ok(separator),
    }) if specifiers.len() == 1 && !f.comments().has_comments(node.syntax()) => {
        write!(f, [space(), node.format()])?;
        
        if let Some(separator) = separator {
            write!(f, [format_removed(&separator)])?;
        }
        
        write!(f, [space()])?;
    }
    _ => {
        if node_has_leading_newline(specifiers.syntax()) {
            write!(f, [block_indent(&specifiers.format()),])?;
        } else {
            write!(
                f,
                [group(&soft_space_or_block_indent(&specifiers.format())),]
            )?;
        };
    }
}

Thanks for pointing to the import specifier code. I noticed that I missed a check when refactoring how Rome handles comments.

Would you mind changing

  if specifier.syntax().has_comments_direct()
      || separator
          .map(|sep| {
              sep.has_leading_comments() || sep.has_trailing_comments()
          })
          .unwrap_or(false)
  {
      write!(f, [named_import.format()])
  } else {

to

 if f.comments().has_comments(specifier.syntax()) {
      write!(f, [named_import.format()])
  } else {

Copy link
Contributor Author

@unvalley unvalley Nov 15, 2022

Choose a reason for hiding this comment

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

@MichaReiser
Thanks for reviewing and clear explanation! I've updated the code.
I also edited import specifier code and its test.

@unvalley unvalley marked this pull request as ready for review November 14, 2022 17:53
@unvalley unvalley requested a review from ematipico as a code owner November 14, 2022 17:53
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.

Thank you!

@MichaReiser MichaReiser added A-Formatter Area: formatter L-JavaScript Langauge: JavaScript labels Nov 16, 2022
@MichaReiser MichaReiser added this to the 11.0.0 milestone Nov 16, 2022
@MichaReiser MichaReiser merged commit c7e93bb into rome:main Nov 16, 2022
@unvalley unvalley deleted the fix/restrict-export-line-break branch November 16, 2022 08:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Formatter Area: formatter L-JavaScript Langauge: JavaScript
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants