Skip to content
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

Merged
merged 4 commits into from
Dec 19, 2023

Conversation

faultyserver
Copy link
Contributor

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:

| {
  // this dangling comment gets duplicated
  [A in B]: T
}// this dangling comment gets duplicated;

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.

Copy link

netlify bot commented Dec 17, 2023

Deploy Preview for biomejs ready!

Name Link
🔨 Latest commit 3dab029
🔍 Latest deploy log https://app.netlify.com/sites/biomejs/deploys/65809cb388ee0400080e1899
😎 Deploy Preview https://deploy-preview-1240--biomejs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 100 (no change from production)
Accessibility: 97 (no change from production)
Best Practices: 100 (no change from production)
SEO: 93 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-actions github-actions bot added A-Formatter Area: formatter L-JavaScript Language: JavaScript and super languages labels Dec 17, 2023
@@ -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"]
Copy link
Contributor Author

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Copy link
Member

@Conaclos Conaclos left a 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 :)

@github-actions github-actions bot added the A-Changelog Area: changelog label Dec 18, 2023
@github-actions github-actions bot added the A-Website Area: website label Dec 18, 2023
@faultyserver faultyserver merged commit 0a8ffd0 into main Dec 19, 2023
20 checks passed
@faultyserver faultyserver deleted the faulty/mapped-type branch December 19, 2023 01:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-CLI Area: CLI A-Formatter Area: formatter A-Website Area: website L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

📝 Comment near a computed key in a union type gets duplicated
3 participants