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): remove conditional expanded content in RemoveSoftLinesBuffer #1032

Merged
merged 3 commits into from
Dec 4, 2023

Conversation

faultyserver
Copy link
Contributor

Summary

In #934 I tried fixing an issue where arrow chains as call arguments wouldn't cause the parent to expand even if the signatures broke over multiple lines. In doing that, I saw that it was using the RemoveSoftLinesBuffer, which should've matched Prettier's behavior, but wasn't working for some reason. So, I introduced ParameterLayout::Compact, which did some additional forcing to remove potential line breaks, and that did the trick.

But, when running Biome on our monorepo, this peculiar issue came up: Playground Link

aLongFunctionName(({ parameter1, parameter2, parameter3, parameter4, and }) => {
        const a = 1;
      }
    );

// Prettier
aLongFunctionName(({ parameter1, parameter2, parameter3, parameter4, and }) => {
  const a = 1;
});

// Biome
aLongFunctionName(
  ({ parameter1, parameter2, parameter3, parameter4, and }) => {
    const a = 1;
  },
);

Biome breaks the call arguments, even though they fit exactly in the line width (80 chars here), while Prettier correctly fits the line.

After some investigation, it turns out this is because the object destructuring pattern was adding an if_group_breaks(",") entry in the doc for the trailing comma on the object pattern. But that entry shouldn't get rendered when the object fits on a single line. I'm not totally sure why it was being printed when checking if the line fits, but that extra character caused it to not fit and forced the parent to expand. Prettier doesn't have this entry in the IR for the same code, so I started looking at that.

Taking a closer look at their removeLinesFn, it also checks for DOC_TYPE_IF_BREAK and forces the flat content to be rendered instead of the expanded content...Ah! Biome's RemoveSoftLinesBuffer was only checking for actual soft line breaks, but it didn't check for this conditional content.

So I added in similar logic to remove any conditional content for when mode == PrintMode::Expanded and...suddenly it all works! That turned out to be the missing piece all along. In fact, it actually solved the previous issue as well, meaning ParameterLayout::Compact wasn't actually necessary.

So, this PR fixes the RemoveSoftLinesBuffer logic to match Prettier, and also removs the ParameterLayout::Compact change from that other PR.

Test Plan

Added a spec test to cover this specific case, and running the test suite didn't cause any other changes to existing snapshots.

Copy link

netlify bot commented Dec 3, 2023

Deploy Preview for rad-torte-839a59 ready!

Name Link
🔨 Latest commit c9d982e
🔍 Latest deploy log https://app.netlify.com/sites/rad-torte-839a59/deploys/656e1d2e26fc350008e212a6
😎 Deploy Preview https://deploy-preview-1032--rad-torte-839a59.netlify.app/playground
📱 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 configuration.

@github-actions github-actions bot added A-Formatter Area: formatter L-JavaScript Language: JavaScript and super languages labels Dec 3, 2023
@Conaclos Conaclos changed the title fix(js_formatter): Remove conditional expanded content in RemoveSoftLinesBuffer fix(js_formatter): remove conditional expanded content in RemoveSoftLinesBuffer Dec 4, 2023
@ematipico
Copy link
Member

Feel free to merge it once the conflicts are resolved

@faultyserver faultyserver merged commit 8801661 into main Dec 4, 2023
18 checks passed
@faultyserver faultyserver deleted the faulty/destructured-arrow-param branch December 4, 2023 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Formatter Area: formatter L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants