-
Notifications
You must be signed in to change notification settings - Fork 653
feat(rome_js_formatter): TS Intersection & Union types #3162 #3177
Conversation
✅ Deploy Preview for docs-rometools ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
if !is_prev_object_type_like && !is_object_type_like { | ||
write!( | ||
f, | ||
[indent(&format_args![ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a difference in IR.
Prettier has a prev &
in indent
for this condition. To achieve this, need to somehow save the previous separating token. it's inconvenient and it seems that both IRs have the same print.
I've tried to extract a &
in prettier code result.push(" &", indent([line, types[i]]));
and all tests passed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any chance you can use align
to get the same behaviour?
Meaning, you use align
to indent by one space, and then write "&` at the start of every line if the group breaks.
This should get you the same result that the printer writes a space followed by &
at the start of every line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm,
Prettier has a strange decision to treat unions and intersections in different ways.
For intersections it prints &
at the end of every line. Can align
help in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so. align
has a very close semantic to indent
. The difference is that align
allows you to specify how many spaces should be printed where indent
always uses the indent sequence specified in the configuration.
align
is necessary to force print whitespace because our printer otherwise omits spaces at the beginning of the line.
My understanding of
indent([" &", line, "BBBBBBBBBBBBBBBBBBBBBB"]),
indent([" &", line, "CCCCCCCCCCCCCCCCCCCCCC"]),
indent([" &", line, "DDDDDDDDDDDDDDDDDDDDDD"]),
Is that prettier will indent the content if the group breaks and then prints the _ &
before the line break.
I don't think we need align
here because this is simply a space between what comes before. And I think we should be able to construct the very same IR even without using align
because the problem here is a space at the end
write!(f, [
indent(&format_args![space(), text("&"), soft_line_break_or_space(), ... ])
])?;
But you'll have to give it a try.
Edit: text("&")
should be the formatted separator token.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting,
I constructed the same IR at first but the solution had a complexity with separator
.
AstSeparatedElement
has a trailing separator. element = [node, &]
indent([" &", line, node])
has a &
token from a previous element.
When constructing this Prettier IR needs to save the previous separator in Option
.
But then I noticed that
indent([" &", line, "BBBBBBBBBBBBBBBBBBBBBB"]),
and
" &", indent([line, "BBBBBBBBBBBBBBBBBBBBBB"]),
had the same output because indent worked after a line break. After extracting a &
from indent
I could write trailing_separator
in current step.
I can push the version with previous separator and Option
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW: prettier/prettier#3986 it seems that Prettier has a plan to change formatting.
crates/rome_js_formatter/tests/specs/prettier/typescript/union/union-parens.ts.snap
Show resolved
Hide resolved
|
||
const foo1 = [abc, def, ghi, jkl, mno, pqr, stu, vwx, yz] as ( | ||
| string | ||
@@ -58,11 +60,11 @@ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, do you know why the formatting differs on line 154...163?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to handle only TsIntersectionType
in this PR. Because Prettier has functions to handle unions and intersections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome. Looks good. Thank you so much for this contribution.
Would you mind adding the updated metrics to the test plan.
You can collect them with
REPORT_PRETTIER=1 cargo test
This will create a report.md
document and the metrics are at the very top. Ideally, run the metrics on main first, then on your PR and include the two metrics for each branch in your test plan.
Summary
Fix #3162 for TS Intersection.
Prettier: https://github.com/prettier/prettier/blob/cd3e530c2e51fb8296c0fb7738a9afdd3a3a4410/src/language-js/print/type-annotation.js#L93-L120
Test Plan
Current:
Average compatibility: 85.39
Compatible lines: 83.81
Main:
Average compatibility: 85.26
Compatible lines: 83.67
New test cases
intersection_type.ts
cargo test --package rome_js_formatter