fix(js_formatter): Use fluid assignment layout when left side is breakable #1021

faultyserver · 2023-12-02T20:44:49Z

Summary

This is an equivalent fix for prettier/prettier#15534, where an assignment with a left-hand side that is breakable inadvertently used the NeverBreakAfterOperator assignment layout, causing it to break before the operator would: Playground Link.

params["redirectTo"] = `${window.location.pathname}${window.location.search}${window.location.hash}`;
// Before:
params[
	"redirectTo"
] = `${window.location.pathname}${window.location.search}${window.location.hash}`;
// After:
params["redirectTo"] =
	`${window.location.pathname}${window.location.search}${window.location.hash}`;

Implementing this required a few additions to match some utilities that Prettier uses, specifically a new may_directly_break function to complement the existing will_break. Where will_break checks if an element or list of elements is guaranteed to break because of its content, the new may_directly_break checks if an element could potentially break, by way of containing a Line element of any kind. The main consideration here is that will_break only considers hard lines, while may_directly_break also accounts for soft lines.

The function is...a little bit strange semantically, just because it's not quite the same type of check as will_break, so I'm not super confident about the naming of it, but it matches can_break in Prettier, which feels close enough I guess.

There are a few other oddities throughout that I'll add comments on directly.

Test Plan

The prettier diff snapshot is deleted because they now match, and no other tests have been affected.

…kable

netlify · 2023-12-02T20:44:54Z

✅ Deploy Preview for rad-torte-839a59 ready!

Name	Link
🔨 Latest commit	`d47bcc2`
🔍 Latest deploy log	https://app.netlify.com/sites/rad-torte-839a59/deploys/656e22e83fbfa40008729fe3
😎 Deploy Preview	https://deploy-preview-1021--rad-torte-839a59.netlify.app/playground
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

faultyserver · 2023-12-02T20:45:38Z

crates/biome_formatter/src/format_element.rs

+    pub const fn is_line(&self) -> bool {
+        matches!(self, FormatElement::Line(_))
+    }


Kinda surprised this didn't exist before. Turns out it's not actually necessary because may_directly_break does this check itself, but I figure this makes sense to keep around anyway.

faultyserver · 2023-12-02T20:46:29Z

crates/biome_formatter/src/format_element/document.rs

+            match element {
+                // Line suffix
+                // Ignore if any of its content breaks
+                FormatElement::Tag(StartLineSuffix) => {
+                    ignore_depth += 1;
+                }
+                FormatElement::Tag(EndLineSuffix) => {
+                    ignore_depth -= 1;
+                }
+                FormatElement::Interned(interned) if ignore_depth == 0 => {
+                    if interned.may_directly_break() {
+                        return true;
+                    }
+                }


I'd like to not duplicate this code again, but idk a cleaner way to write it out. I think this state management is still necessary for this function, since it also shouldn't be inspecting text content on suffixes, but I'm not 100% sure.

I think it's fine :) the duplicated code isn't that much

faultyserver · 2023-12-02T20:49:53Z

crates/biome_js_formatter/src/utils/format_modifiers.rs

+        if self.list.is_empty() {
+            return Ok(());
+        }


Returning early here is important, because otherwise this node returns a group that always has a soft_line_break_or_space, which causes may_directly_break to return true. An example case this affects is:

class Test { prop1 = // comment true; }

Without this early escape, the IR gets written as:

group(expand: propagated, [ group(expand: true, [soft_line_break_or_space]), " prop1", line_suffix([" // comment"]), expand_parent ]), " = true"

and that group with just the soft line is this modifier list. With this early return, that group no longer exists, meaning the outer group won't contain any potential breaks, and the comments get formatted as expected by Prettier.

This is a great comment, and a I think we should have it inside the code :) (maybe a variation of that)

ematipico

Happy to merge it once we add code comment that explains the empty list :)

ematipico · 2023-12-04T17:56:21Z

crates/biome_formatter/src/format_element.rs

+    /// such as when the group contains a [crate::builders::expand_parent] or some text within the group
+    /// contains a newline. Neither of those cases directly contain a [FormatElement::Line], and so they
+    /// do not _directly_ break.
+    fn may_directly_break(&self) -> bool;


I don't mind the name of the function, but I wonder if at this point, we should rename will_break in will_inderectly_break.

Honestly I don't know which is the better name between will_break, will_directly_break or will_indirectly_break...because they're all both accurate and inaccurate in different ways lol. indirectly i think implies that the element won't break itself, but some child element does, or something like that. directly implies that the element itself does, but children might also break instead. the plain will_break at least covers both of those, despite being a little more ambiguous to read.

You're right. I hope to find better names , because the current ones aren't very good and the prettier code base doesn't help

ematipico · 2023-12-04T17:59:32Z

crates/biome_formatter/src/format_element/document.rs

+            match element {
+                // Line suffix
+                // Ignore if any of its content breaks
+                FormatElement::Tag(StartLineSuffix) => {
+                    ignore_depth += 1;
+                }
+                FormatElement::Tag(EndLineSuffix) => {
+                    ignore_depth -= 1;
+                }
+                FormatElement::Interned(interned) if ignore_depth == 0 => {
+                    if interned.may_directly_break() {
+                        return true;
+                    }
+                }


I think it's fine :) the duplicated code isn't that much

ematipico · 2023-12-04T18:00:58Z

crates/biome_js_formatter/src/utils/format_modifiers.rs

+        if self.list.is_empty() {
+            return Ok(());
+        }


This is a great comment, and a I think we should have it inside the code :) (maybe a variation of that)

fix(js_formatter): Use fluid assignment layout when left side is brea…

8206d7e

…kable

github-actions bot added A-Formatter Area: formatter L-JavaScript Language: JavaScript and super languages labels Dec 2, 2023

faultyserver commented Dec 2, 2023

View reviewed changes

ematipico self-requested a review December 3, 2023 01:12

ematipico approved these changes Dec 4, 2023

View reviewed changes

faultyserver and others added 2 commits December 4, 2023 19:03

add comment for early return

692d439

Merge branch 'main' into faulty/assignment-breaks

d47bcc2

faultyserver merged commit 1988900 into main Dec 4, 2023
18 checks passed

faultyserver deleted the faulty/assignment-breaks branch December 4, 2023 21:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(js_formatter): Use fluid assignment layout when left side is breakable #1021

fix(js_formatter): Use fluid assignment layout when left side is breakable #1021

faultyserver commented Dec 2, 2023

netlify bot commented Dec 2, 2023 •

edited

Loading

faultyserver Dec 2, 2023

faultyserver Dec 2, 2023

ematipico Dec 4, 2023

faultyserver Dec 2, 2023

ematipico Dec 4, 2023

ematipico left a comment •

edited

Loading

ematipico Dec 4, 2023

faultyserver Dec 4, 2023

ematipico Dec 4, 2023

ematipico Dec 4, 2023

ematipico Dec 4, 2023

fix(js_formatter): Use fluid assignment layout when left side is breakable #1021

fix(js_formatter): Use fluid assignment layout when left side is breakable #1021

Conversation

faultyserver commented Dec 2, 2023

Summary

Test Plan

netlify bot commented Dec 2, 2023 • edited Loading

✅ Deploy Preview for rad-torte-839a59 ready!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ematipico left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Dec 2, 2023 •

edited

Loading

ematipico left a comment •

edited

Loading