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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b64f8c1
fix(rome_js_formatter): restrict export line break for single specifier
unvalley Nov 14, 2022
f05ad09
refactor: handle specifeirs format by its length
unvalley Nov 14, 2022
a373f4d
fix: format single specifier without comma separator
unvalley Nov 14, 2022
ac8f2e8
test: add long string single specifier to export and update snapshot
unvalley Nov 14, 2022
d50a47f
test: update prettier_test snapshot
unvalley Nov 14, 2022
27a2f1c
fix: handle single specifier with no comments
unvalley Nov 15, 2022
e99a13a
Update crates/rome_js_formatter/tests/specs/js/module/export/named_fr…
unvalley Nov 15, 2022
4318d1c
Merge branch 'fix/restrict-export-line-break' of https://github.com/u…
unvalley Nov 15, 2022
510737e
test: update snapshot test
unvalley Nov 15, 2022
6d1db14
fix: handling comments for import named clause
unvalley Nov 15, 2022
c2042dc
test: add snapshot test for import specifier
unvalley Nov 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 22 additions & 8 deletions crates/rome_js_formatter/src/js/module/export_named_from_clause.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,28 @@ impl FormatNodeRule<JsExportNamedFromClause> for FormatJsExportNamedFromClause {

write!(f, [l_curly_token.format(),])?;

if node_has_leading_newline(specifiers.syntax()) {
write!(f, [block_indent(&specifiers.format()),])?;
} else {
write!(
f,
[group(&soft_space_or_block_indent(&specifiers.format())),]
)?;
};
match specifiers.len() {
1 => {
// SAFETY: we know that `specifiers().len() == 1`, so unwrap `iter().next()` is safe.
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.

tools/crates/rome_js_formatter/src/js/module/import_named_clause.rs

Line 58 in 58fbb93

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

Copy link
Contributor

@MichaReiser MichaReiser Nov 15, 2022 •
edited
Loading

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 •
edited
Loading

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.

write!(f, [space(), specifier.format(), space()])?;
}
_ => write!(f, [&specifiers.format()])?,
}
}
_ => {
if node_has_leading_newline(specifiers.syntax()) {
write!(f, [block_indent(&specifiers.format()),])?;
} else {
write!(
f,
[group(&soft_space_or_block_indent(&specifiers.format())),]
)?;
};
}
}

write![
f,
Expand Down
5 changes: 4 additions & 1 deletion crates/rome_js_formatter/tests/specs/js/module/export/named_from_clause.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,7 @@ export {a,
export {
lorem_lorem_lorem_lorem_lorem_lorem_lorem_lorem_lorem,
lorem_lorem_lorem_lorem_lorem_ as ipsum_ipsum_ipsum_ipsum_ipsum_ipsum_
} from "fancy" assert { type: "json", "type2": "json", type23: "json", "type24": "json"}
} from "fancy" assert { type: "json", "type2": "json", type23: "json", "type24": "json"}


export { loooooooooooooooooooooooooooooooooooooooooooooooooong } from "loooooooooooooooooooooooooooooooooooooooooooooong"
10 changes: 10 additions & 0 deletions crates/rome_js_formatter/tests/specs/js/module/export/named_from_clause.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ export {
lorem_lorem_lorem_lorem_lorem_lorem_lorem_lorem_lorem,
lorem_lorem_lorem_lorem_lorem_ as ipsum_ipsum_ipsum_ipsum_ipsum_ipsum_
} from "fancy" assert { type: "json", "type2": "json", type23: "json", "type24": "json"}


export { loooooooooooooooooooooooooooooooooooooooooooooooooong } from "loooooooooooooooooooooooooooooooooooooooooooooong"
```


Expand Down Expand Up @@ -44,6 +47,13 @@ export {
type23: "json",
"type24": "json",
};

export { loooooooooooooooooooooooooooooooooooooooooooooooooong } from "loooooooooooooooooooooooooooooooooooooooooooooong";


## Lines exceeding width of 80 characters

13: export { loooooooooooooooooooooooooooooooooooooooooooooooooong } from "loooooooooooooooooooooooooooooooooooooooooooooong";
```


21 changes: 10 additions & 11 deletions crates/rome_js_formatter/tests/specs/prettier/js/export-extension/export.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,16 +23,12 @@ export { foooooooooooooooooooooooooooooooooooooooooooooo, fooooooooooooooooooooo
```diff
--- Prettier
+++ Rome
@@ -1,12 +1,31 @@
@@ -1,12 +1,29 @@
export * as ns from "mod";
-export v from "mod";
-export a, * as b from "mod";
-export c, { foo } from "mod";
-export * as d, { bar } from "mod";
-export { fooooooooooooooooooooooooooooooooooooooooooooooooo } from "fooooooooooooooooooooooooooooo";
-export Bar, {
- barrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrr,
-} from "barrrrrrrrrrrrrrrrrrrrrrrrrrrr";
+export
+v;
+from;
Expand All @@ -53,9 +49,10 @@ export { foooooooooooooooooooooooooooooooooooooooooooooo, fooooooooooooooooooooo
+}
+from;
+("mod");
+export {
+ fooooooooooooooooooooooooooooooooooooooooooooooooo,
+} from "fooooooooooooooooooooooooooooo";
export { fooooooooooooooooooooooooooooooooooooooooooooooooo } from "fooooooooooooooooooooooooooooo";
-export Bar, {
- barrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrr,
-} from "barrrrrrrrrrrrrrrrrrrrrrrrrrrr";
+export
+Bar, { barrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrr };
+from;
Expand Down Expand Up @@ -89,9 +86,7 @@ export * as d
}
from;
("mod");
export {
fooooooooooooooooooooooooooooooooooooooooooooooooo,
} from "fooooooooooooooooooooooooooooo";
export { fooooooooooooooooooooooooooooooooooooooooooooooooo } from "fooooooooooooooooooooooooooooo";
export
Bar, { barrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrr };
from;
Expand Down Expand Up @@ -490,4 +485,8 @@ export.js:7:68 parse ━━━━━━━━━━━━━━━━━━━

```

# Lines exceeding max width of 80 characters
```
22: export { fooooooooooooooooooooooooooooooooooooooooooooooooo } from "fooooooooooooooooooooooooooooo";
```