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): fix array fill elements expanding from leading comments #1135

Merged
merged 3 commits into from
Dec 10, 2023

Conversation

faultyserver
Copy link
Contributor

@faultyserver faultyserver commented Dec 9, 2023

Summary

This one was a doozy to figure out. The root of the issue is printing a concise array with an own-line comment:

const numbers1 = [
  -2017,
  // comment1
  -380014, -253951682, -214
];

We want the formatted output to look exactly like this, but it was actually being printed as:

const numbers1 = [
  -2017,
  // comment1
  -380014,
-253951682, -214
];

Where the entry with the leading comment got its own line, and then the fill continued on the following line.

This was somewhat intended, as noted by this comment in a test, but it doesn't match the behavior of Prettier, which does preserve the exact formatting as the input.

The cause of this difference was how FormatElement::Line(LineMode::Hard) got handled when measuring fit. It had been checking must_be_flat and returning No if there was a hard line when must_be_flat was true. This makes sense, as that comment above mentions, because the content doesn't fit on a single line. Prettier, however, doesn't care about whether the fit must be flat, and just always considered hard line breaks as fitting. This also makes sense, because the print is guaranteed to not exceed the line width, which is the real purpose of fits.

Fixing this original issue was then just a matter of removing the check for must_be_flat when a hard line is encountered. That (plus a change to the generated IR for array fills to use a hardline separator before the leading comment) made these tests for array fills match Prettier.

However, this caused a different issue with JSX elements, where now multiline elements that hadn't been considered "fitting" before now did fit and caused the separators to print in flat mode when they shouldn't, like so:

<div>
  <span a b>
    <Hi />
  </span> ({variable})
</div>;

In this case, because <span>...</span> contains hard lines and goes over multiple lines, we don't want it to count as a "flat" element, and instead it should force expansion like it did before (and matching Prettier), yielding:

<div>
  <span a b>
    <Hi />
  </span>{" "} // <- This separator is `if_group_breaks('{" "}')`
  ({variable})
</div>;

We know why this changed (because hard lines weren't considered fitting before and now they are), but if that behavior now matches Prettier, why does this result in the JSX not? After much attempted debugging of the printer to try to find other differences about how fits_element was working, it turned out to just be that the generated IR for these multiline children was different.

For any multiline JSX element, Prettier wraps the content with a group. This group gets automatically expanded by propagation from the hardline breaks contained within it, and so when the fits check happens, the group says "nope, I definitely expand, so I don't fit in flat mode" and returns false. Because of that, the fill element expands and the separator also gets printed in expanded mode (matching the if_group_breaks condition as mentioned in the comment above).

But Biome wasn't wrapping the content with a group. That meant that the hardline breaks weren't propagated to anything within the Fill, and it didn't know that it shouldn't be considered fitting. By adding the group around the JSX children, it now knows that there is a line break, shouldn't fit, and should expand both the item and the separator, exactly the way that Prettier does.

Test Plan

Prettier diff snapshots are deleted since they now match, and no other tests were affected.

I did remove the prettier difference spec test since the behavior now matches Prettier.

Copy link

netlify bot commented Dec 9, 2023

Deploy Preview for biomejs ready!

Name Link
🔨 Latest commit 1ba3dcd
🔍 Latest deploy log https://app.netlify.com/sites/biomejs/deploys/6574e3ee7e0b6f0008165910
😎 Deploy Preview https://deploy-preview-1135--biomejs.netlify.app
📱 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 9, 2023
Comment on lines +116 to +118
LineMode::Hard | LineMode::Empty => {
self.state.measured_group_fits = false;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only actual change in the whole FormatElement::Line match arm. The rest is just refactoring to make sure this case gets hit at the right time, and the diff is just whitespace

@Conaclos Conaclos merged commit 3262574 into main Dec 10, 2023
18 checks passed
@Conaclos Conaclos deleted the faulty/array-number-comments branch December 10, 2023 12:28
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.

None yet

2 participants