fix(js_formatter): don't duplicate dangling comments for mapped types#1240
Merged
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
faultyserver
commented
Dec 17, 2023
| // ├── symlink_testcase1_1 -> hidden_nested | ||
| // └── symlink_testcase2 -> hidden_testcase2 | ||
| #[test] | ||
| #[ignore = "It regresses on linux since we added the ignore crate, to understand why"] |
Contributor
Author
There was a problem hiding this comment.
@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).
Conaclos
approved these changes
Dec 18, 2023
Conaclos
left a comment
Member
There was a problem hiding this comment.
Looks good to me! Feel free to merge once you added a changelog entry :)
ematipico
approved these changes
Dec 18, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
TsUnionTypeformats 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-modetests 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.