-
-
Notifications
You must be signed in to change notification settings - Fork 540
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(js_formatter): don't duplicate dangling comments for mapped types #1240
Conversation
✅ Deploy Preview for biomejs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
…, also fix break mode test
22bed7a
to
2325bcc
Compare
@@ -992,6 +992,7 @@ fn fs_error_unknown() { | |||
// ├── symlink_testcase1_1 -> hidden_nested | |||
// └── symlink_testcase2 -> hidden_testcase2 | |||
#[test] | |||
#[ignore = "It regresses on linux since we added the ignore crate, to understand why"] |
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.
@ematipico I see you added a similar ignore in commands/check.rs
, but it looks like this one in lint
can also fail on linux, so I'm adding the same one here, unless you think there's another solution or this is actually something incorrect (but since this is just a formatter change, i don't imagine it is).
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.
Thank you!
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.
Looks good to me! Feel free to merge once you added a changelog entry :)
Summary
Fixes #1220. The base case issue here was that dangling comments of mapped types within a union type were getting printed twice, but another portion was that they were positioned incorrectly in the first place. Consider:
The comment is printed twice, so let's fix that first. The root cause was that
TsUnionType
formats all of the comments of each variant directly. All leading, dangling, and trailing. For most nodes this isn't a problem since they don't do any special handling for them. But for mapped types, the dangling comments are directly formatted by the node itself (and moved to act as leading coments instead), so when the union then formatted the comments as dangling comments, they were printed twice. The same was actually also true for empty object-like and tuple types, which also format their own dangling comments.The solution here was just to opt-out of re-printing the comments in the union for those types of nodes. While working in there, I also copied over the rest of prettier's logic for trailing comments in mapped types as well.
This PR also addresses the
break-mode
tests from Prettier, where the mapped type should only break over multiple lines if a newline occurs between the{
and the start of the property name. Newlines in any other position don't cause the type to expand unless the inner groups break.Test Plan
The Prettier snapshots cover both of these cases, break mode for itself, and 11098 for the comment positioning.