fix(js_formatter): Match Prettier's breaking strategy for ArrowChain layouts #934
Conversation
faultyserver
temporarily deployed
to
Website deployment
GitHub Actions
Inactive
github-actions
bot
added
A-Formatter
|
!bench_formatter |
| .and_then(FormatAnyJsParameters::cast) | ||
| .and_then(JsParameters::cast) | ||
| .map_or(false, |parameters| { | ||
| should_hug_function_parameters(¶meters, f.comments()).unwrap_or(false) | ||
| should_hug_function_parameters( | ||
| &FormatAnyJsParameters::new( | ||
| AnyJsParameters::JsParameters(parameters), | ||
| FormatJsParametersOptions::default(), | ||
| ), | ||
| f.comments(), | ||
| ) | ||
| .unwrap_or(false) |
There was a problem hiding this comment.
there might be a more Rust-y way to do this, but i'm not at all sure. it can't just be cast any more since the options field of the struct now has to be specified.
| pub(crate) struct FormatAnyJsParameters { | ||
| pub(crate) parameters: AnyJsParameters, | ||
| pub(crate) options: FormatJsParametersOptions, | ||
| } | ||
|
|
||
| impl FormatAnyJsParameters { | ||
| pub(crate) fn new(parameters: AnyJsParameters, options: FormatJsParametersOptions) -> Self { | ||
| Self { | ||
| parameters, | ||
| options, | ||
| } | ||
| } |
There was a problem hiding this comment.
I copied most of this from FormatTemplateElement, as another example of a formatting node that takes in options for a union of node types.
| if !parentheses_not_needed { | ||
| write!(f, [l_paren_token.format()])?; | ||
| } else { | ||
| write!(f, [format_removed(&l_paren_token)])?; | ||
| } |
There was a problem hiding this comment.
Seems like we could probably move these paren token writes outside of the match and reduce duplication, but not gonna do that for now.
| let should_hug = | ||
| is_test_call_argument(arrow.syntax())? || is_first_or_last_call_argument; |
There was a problem hiding this comment.
Because single parameters and lists are handled separately, the group layout condition needs to be checked here, and it was easy enough to just tack onto the hug condition.
| vec![middle_variant, most_expanded.into_boxed_slice()] | ||
| } else { | ||
| vec![most_flat, middle_variant, most_expanded.into_boxed_slice()] |
There was a problem hiding this comment.
not sure why, but the best_fitting! macro really doesn't like playing along here with Box<[FormatElement]>, otherwise i'd prefer to use that than the unsafe block below.
faultyserver
temporarily deployed
to
Website deployment
GitHub Actions
Inactive
Formatter Benchmark Results |
|
i'm actually a little astounded that this didn't affect the formatter performance more. But that was really my one worry, so i'm happy with this lol |
|
Ended up reading a few different parts of the code as I started looking at some other things to fix and realized there is actually a |
faultyserver
temporarily deployed
to
Website deployment
GitHub Actions
Inactive
|
I did some exploration into using So I'm going to leave it as is for now. I think there's a larger refactor that can come here to switch up the branching hierarchy and reduce duplication, but that'll definitely be something to tackle later instead. |
Summary
This introduces a new
ParameterLayout::Compactto force parameters to print with no line breaks at all (through a newprevent_soft_line_breaksoption on the formatting node), helping theFormatJsCallArgumentsnode properly test the fit of chained/curried arrow functions.In short:
Longer Playground Link
Background
After spending an entire week trying to guess how the printer for Biome was acting different from Prettier's for what seemed like the same content, I eventually landed on the fact that Biome organizes arrow chains as a flat list of signatures followed by a single body, while Prettier just handles them recursively (each arrow is the body of the previous arrow).
I messed around for a while trying to understand what that meant and learning how the Printer works and how groups and line breaks get measured along with how
BestFittingworks, all to no avail, but finally realized that the primary difference between Biome and Prettier is that Prettier can remove all line breaks from a doc sequence when trying to format things in a flat manner. Biome can't currently do this, and was usingsoft_line_break_or_space()in a few places where Prettier ended up forcing a simple space character instead.As a result, when Biome would use
BestFittingto try to layout arguments in a call expression, it would conveniently find these line breaks and split there, stating that the group fits because the break happened before reaching the maximum width.This meant that even though the arrow expression would have to break across multiple lines, it wouldn't actually cause the parent call expression to expand, resulting in a weird mixed of flat and expanded layout.
While the logic for comparing "most flat", "middle", and "most expanded" variants for call arguments matched Prettier, it was this inadvertent line breaking that was causing the issues. The crux of Prettier's logic is that the chain of arrow signatures either must fit on a single line. Then, the "most flat" variant tries to format the tail body as a single line, and the "middle" variant allows the body to break onto multiple lines. Importantly, though, the signatures must still fit on a single line. If they still don't, then the middle variant also fails, and the printer falls back to the most expanded layout.
By preventing those line breaks from being inserted in the parameter list (using a new
ParameterLayout::Compact), the Printer now appropriately realizes that the arrow signature chain doesn't fit on a single line and forces it to fall back to the most expanded, just like Prettier.A few other changes also have to be made to accommodate that behavior and not affect any other formatting logic, just around how exactly parameters and spaces are grouped, all of which just brings the resulting IR closer to how Prettier renders these elements, while still retaining the flat structure for the whole arrow chain.
Test Plan
The Prettier diff snapshots are deleted, since the output now matches Prettier, and no other tests were inadvertently updated.