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): ensure flat conditional content is always written in RemoveSoftLinesBuffer #1357

Merged
merged 3 commits into from
Dec 28, 2023

Conversation

faultyserver
Copy link
Contributor

Summary

Fixes #1356. I don't feel like this should've actually solved the issue, but it was still an underlying trap in RemoveSoftLinesBuffer that conditional content was still conditional. Technically that is correct for what the name of the buffer implies, but the actual functionality that it's going for is "force all elements to print in flat mode", not just "remove lines". By keeping the conditional in place, it'd be possible for a group to break and cause the if_group_fits_on_line condition to fail, omitting the content, even though the intent is to just print that content and assume that it fits. Or rather, the goal is to test if it fits in the context of the rest of the content being printed in the buffer, normally as part of something like best_fitting!.

This was exactly the case that happened in #1356, where the content was just long enough that the group containing the object type annotation wouldn't fit and ended up breaking, and because the semicolon was optional, it ended up being omitted as part of the if_group_fits_on_line condition. Now that the condition is elided and the flat content is just printed no matter what, the semicolon will always render regardless of whether the group ends up breaking, and the result comes out as expected (at least, matching Prettier).

For some reason this doesn't feel like the intuitive solution to me. It feels more like there is some logic in calculating fit that's incorrect, but knowing my gut, that's likely not the case and this is the real solution.

Test Plan

Added a spec test specifically covering the issue case, ensuring that the semicolons still get printed.

Copy link

netlify bot commented Dec 28, 2023

Deploy Preview for biomejs ready!

Name Link
🔨 Latest commit ff688ce
🔍 Latest deploy log https://app.netlify.com/sites/biomejs/deploys/658d667b042940000801028c
😎 Deploy Preview https://deploy-preview-1357--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 28, 2023
@github-actions github-actions bot added A-Website Area: website A-Changelog Area: changelog labels Dec 28, 2023
@faultyserver faultyserver merged commit c212dd6 into main Dec 28, 2023
20 checks passed
@faultyserver faultyserver deleted the faulty/fix-1356 branch December 28, 2023 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog 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.

A semicolon is accidentally removed
2 participants