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

feat(rome_js_formatter): TS Intersection & Union types #3162 #3177

Merged
merged 3 commits into from
Sep 8, 2022

Conversation

denbezrukov
Copy link
Contributor

@denbezrukov denbezrukov commented Sep 7, 2022

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

@netlify
Copy link

netlify bot commented Sep 7, 2022

Deploy Preview for docs-rometools ready!

Name Link
🔨 Latest commit c094167
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/6318b4d3f651e600082bc8f6
😎 Deploy Preview https://deploy-preview-3177--docs-rometools.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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![
Copy link
Contributor Author

@denbezrukov denbezrukov Sep 7, 2022

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.

Copy link
Contributor

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.

Copy link
Contributor Author

@denbezrukov denbezrukov Sep 8, 2022

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?

Playground

Copy link
Contributor

@MichaReiser MichaReiser Sep 8, 2022

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.

Copy link
Contributor Author

@denbezrukov denbezrukov Sep 8, 2022

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.

Copy link
Contributor Author

@denbezrukov denbezrukov Sep 8, 2022

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.

@denbezrukov denbezrukov requested a review from a team September 7, 2022 14:45

const foo1 = [abc, def, ghi, jkl, mno, pqr, stu, vwx, yz] as (
| string
@@ -58,11 +60,11 @@
Copy link
Contributor

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?

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 want to handle only TsIntersectionType in this PR. Because Prettier has functions to handle unions and intersections.

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.

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.

@MichaReiser MichaReiser merged commit 6bcf6e8 into rome:main Sep 8, 2022
@denbezrukov denbezrukov deleted the feature/ts-intersection branch September 8, 2022 14:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Formatter: TS Intersection & Union types
2 participants
点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载