New primitive to fill a line with as many doc parts as possible #1120

karl · 2017-04-05T12:12:14Z

This PR adds a new fill primitive for laying out documents. This will attempt to fill a line with as many parts of the document as will fit before moving to a new line.

The idea for fill comes from page 14 of Wadler's paper: http://homepages.inf.ed.ac.uk/wadler/papers/prettier/prettier.pdf

If this works it could be a possible fix for #963.

I've aimed for as minimal a change as possible. We keep the existing JSX behaviour of respecting existing new lines. This means that the pretty printing is one way. Once we wrap text we won't ever unwrap it. This seemed like a sensible starting point!

Input:

// Wrapping text
x = <div>
  Some text that would need to wrap on to a new line in order to display correctly and nicely
</div>

// Wrapping tags
x = <div>
  <first>f</first> <first>f</first> <first>f</first> <first>f</first> <first>f</first> <first>f</first>
</div>

// Wrapping tags
x = <div>
  <first>f</first><first>f</first><first>f</first><first>f</first><first>f</first><first>f</first>
</div>

// Wrapping tags
x = <div>
  <a /><b /><c />
  <first>aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa</first> <first>f</first>
</div>

// Wrapping tags
x = <div>
  <sadashdkjahsdkjhaskjdhaksjdhkashdkashdkasjhdkajshdkashdkashd /> <first>f</first>
</div>

Output:

// Wrapping text
x = (
  <div>
    Some text that would need to wrap on to a new line in order to display
    correctly and nicely
  </div>
);

// Wrapping tags
x = (
  <div>
    <first>f</first> <first>f</first> <first>f</first> <first>f</first>
    {" "}
    <first>f</first> <first>f</first>
  </div>
);

// Wrapping tags
x = (
  <div>
    <first>f</first><first>f</first><first>f</first><first>f</first>
    <first>f</first><first>f</first>
  </div>
);

// Wrapping tags
x = (
  <div>
    <a /><b /><c />
    <first>aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa</first>
    {" "}
    <first>f</first>
  </div>
);

// Wrapping tags
x = (
  <div>
    <sadashdkjahsdkjhaskjdhaksjdhkashdkashdkasjhdkajshdkashdkashd />
    {" "}
    <first>f</first>
  </div>
);

vjeux · 2017-04-05T15:58:38Z

Awesome! @rattrayalex could you take a look to see if that's what you had in mind?

karl · 2017-04-05T16:00:37Z

I've just pushed my latest prototyping, and written up a list of important things that still need to be worked on.

It's taken me most of the day to get my head around Walder's paper and the Prettier code base, so I haven't had a chance to tackle the many edge cases I can see will occur with JSX and whitespace!

rattrayalex · 2017-04-05T18:08:13Z

wraping my head around this now...

rattrayalex

It looks like this is in fairly early stages, and I don't fully grok it yet, so I don't want to jump to any conclusions, but I'm a bit leery so far... seems fairly JSX-specific.

@vjeux @jlongster as a broader question, is flattening something we hope to bring to strings? It'll obviously involve AST-modification, but could be very nice.

If so, it might be worth starting there (since it'll be harder) or trying to build for both from the get-go.

rattrayalex · 2017-04-05T18:09:39Z

src/doc-printer.js

+            // console.log('only first fits', [first, second]);
+            cmds.push(remainingCmd)
+            if (typeof second !== 'string' || typeof first !== 'string') {
+              cmds.push([ind, MODE_BREAK, "{' '}"]);


woah, this kind of JSX-specific stuff is definitely concerning... perhaps it could come out prior to release...

Yep, this felt strange but it was the easiest way to get something sane printing out!

Ideally the fill implementation would be reasonably agnostic, but I don't know enough to make that work yet.

rattrayalex · 2017-04-05T18:13:00Z

tests/jsx-significant-space/__snapshots__/jsfmt.spec.js.snap

-  <span><Icon icon=\\"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\\" /> <Icon icon=\\"\\" /></span>;
-
-not_broken_end =
+"x =


I'm curious to see what the output is on the existing tests... you're welcome to add a new test fixture while you're developing to avoid running on multiple fixtures.

I know the work so far is pretty preliminary, but any chance you'd be willing to run w/ changes to existing tests too?

I've push up a new commit with all the original tests cases (and my single new test case), including all the change snapshots.

Lots has changed! It looks like most of the changes are due to collapsing newlines in JSX, but there are plenty of bugs as well 😛

rattrayalex · 2017-04-05T18:16:25Z

src/printer.js

+        //   words.push(softline);
+        // }
+
+        value.replace(/^\s+|\s+$/g, "").split(/(\s+)/).forEach(word => {


I haven't looked at this super in-depth, but it looks like the intent here would be to squash jsx+element mixtures to be as tight as possible?

So,

<div> hello world </div>

would become

<div> hello world </div>

and

<div> hello <world /> </div>

would be come

<div> hello<world /> </div>

?

(maybe you haven't thought this far ahead yet, which is fine – or maybe you plan to go back to the commented-out code soon enough)

I don't really have an intent at the moment, just playing around with what is possible 😀

Currently it is preserving new lines within JSX text, but I expect it may compact the join between text and tags.

I was hoping after a period of playing around we'd know enough about the possibilities to come up with a coherent set of layout rules that could then be implemented.

rattrayalex · 2017-04-05T18:17:08Z

src/doc-printer.js

          cmds.push([ind, doc.break ? MODE_BREAK : mode, doc.contents]);

          break;
+        case "fill":
+          // include number of spaces
+          cmds.push([ind, mode, " ".repeat(doc.parts.length - 1)]);


This definitely confuses me, but I'm not super used to the printer.

rattrayalex · 2017-04-05T18:17:43Z

src/doc-printer.js

+          const firstCmd = [ind, MODE_FLAT, first];
+          const remainingCmd = [ind, MODE_BREAK, fill(remaining)];
+
+          const firstAndSecondCmd = [ind, MODE_FLAT, concat([first, second])];


I assume the firstAndSecond gambit is primarily intended to understand how to split between text and elements?

The basic idea is copied from fill implementation in Wadler's paper (at the end of page 14).

I believe the idea is to check that the first two elements with both fit on a single line before joining them with a space.

If they don't fit on a single line then we need to join them with a new line.

ah, gotcha, interesting... thanks!

karl · 2017-04-05T19:15:24Z

Thanks for taking a look @rattrayalex, very brave jumping in at this early stage 🙇🏻

I'm likely to be working on this in a very on/off fashion over the next few weeks, so don't feel in any hurry with giving feedback. High level approaches/direction stuff if definitely the most useful while I wrap my head around the algorithms/code.

Also, I'm totally happy if this doesn't end up going anywhere. I'm already learning a lot about JSX and pretty printing 😀

rattrayalex · 2017-04-06T02:46:40Z

cool, good luck & happy hacking! feel free to reach out, I'd be happy to help!

jlongster · 2017-04-07T16:31:52Z

is flattening something we hope to bring to strings? It'll obviously involve AST-modification, but could be very nice.

It sounds neat, but my argument against it is that we never break strings by splitting one string into multiple (and converting into multiple strings with +). We should try to be as symmetrical as possible. Because otherwise it'll collapse something, then you'll add something to it, and it doesn't break it again. The biggest place where this happens now is objects, since once it breaks, it'll never be collapsed again since we keep it expanded. This has caused several stabilization issues since running prettier twice will see the code differently (the first run there were no hard lines, second run there was).

jlongster · 2017-04-07T16:32:45Z

This PR looks really neat, thanks for the work! I don't have time to take a close look right now, but this would be great for JSX.

karl · 2017-04-20T08:31:27Z

I've managed to get this PR into a reviewable state! 😄

After much time spent wrapping my head around the many different ways of handling whitespace in JSX (so much respect for @rattrayalex having already tamed this!) I finally have a small and reasonably sane approach to filling a line with JSX children.

The new fill primitive is now fully generic, so it should be easy to use it for other parts of the document (I think I recall @vjeux wanted to use it for binary expressions?).

I would be great if someone with a little more experience with the code could look at the changes I've made to doc-printer.js. Specifically importing and calling the doc builders. It works nicely, but a sanity check would be great!

rattrayalex

I think this looks like really exciting progress! Seems like a generally correct algorithm with only a few minor issues/concerns from my end.

rattrayalex · 2017-04-20T16:03:14Z

tests/jsx-significant-space/__snapshots__/jsfmt.spec.js.snap

-    {" "}
+    <span
+      barbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbar
+    />{" "}


Hmm, this particular case looks like a regression to me... may not be a big deal.

rattrayalex · 2017-04-20T16:03:39Z

tests/jsx-significant-space/__snapshots__/jsfmt.spec.js.snap

    foo
  </span>
 );

 after_break = (
  <span>
-    foo
-    {" "}
+    foo{" "}


I think the existing behavior was intentionally added by @vjeux (IIRC, it used to behave as you have it for jsx whitespace)

rattrayalex · 2017-04-20T16:04:23Z

tests/jsx-significant-space/__snapshots__/jsfmt.spec.js.snap

@@ -146,13 +145,15 @@ nest_plz = (

 regression_not_transformed_1 = (
  <span>
-    {" "}<Icon icon="aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" />
+    {" "}


I find this surprising... should mirror the reverse cases, no?

(eg; similar_2 below)

rattrayalex · 2017-04-20T16:04:59Z

tests/jsx-significant-space/__snapshots__/jsfmt.spec.js.snap

-    {" "}
-    long text long text
+    long text long text long text long text long text long text long text long
+    text <link>url</link> long text long text


🎉 🎉 🎉

rattrayalex · 2017-04-20T16:05:27Z

tests/jsx-split-attrs/__snapshots__/jsfmt.spec.js.snap

@@ -179,6 +178,7 @@ leave_opening = (
    submitLabel="Sign in with Google"
  >
    {" "}
+


rattrayalex · 2017-04-20T16:19:03Z

src/doc-printer.js

+            break;
+          }
+
+          const split = parts[1];


I find the name split confusing – is it assumed to always be a whitespace type (line, softline, etc)?

might be nice to have a mirroring splitCmd variable here, I found firstCmd somewhat helpful

ahh, I think I see... code like this might really clarify things for me as a reader...

const split = parts[1]; assert.ok(split.type === "line" || split.type === "..."); const splitFlatCmd = [ind, MODE_FLAT, split]; const splitBreakCmd = [ind, MODE_BREAK, split];

I think technically that's a bit less performant in some cases because not all scenarios require the instantiation of both the Flat and Break cmd's, but the overhead should be quite low.

I'm also not sure this will properly account for types like break-parent, line-suffix, line-suffix-boundary, etc – which may not appear in the JSX context but are widespread elsewhere. Perhaps that's not worth building for at this point.

rattrayalex · 2017-04-20T16:21:00Z

src/doc-printer.js

+            break;
+          }
+
+          const second = parts[2];


second seems confusing... it seems to be the third part?

rattrayalex · 2017-04-20T16:22:37Z

src/doc-printer.js

+          }
+
+          const second = parts[2];
+          const remaining = parts.slice(2);


do you mean parts.slice(3)?

karl · 2017-04-20T21:26:23Z

Thanks for the great feedback @rattrayalex!

It might be a week or so before I get to work on this again, but when I do I'll aim at fixing up the regressions and edge cases.

Your right that the fill algorithm could do with a bit of reworking, I was so happy I got it working I submitted the PR without refactoring it 😀

rattrayalex · 2017-04-20T21:32:44Z

Awesome! I mean I think what you have is really great, and already very readable (all things considered) 😉

karl · 2017-04-21T21:57:57Z

@rattrayalex I've pushed a hacked together quick fix that (I think) fixes all the regressions 😄

The code could probably do with a tidy up before merging, but I think the behaviour is looking good!

rattrayalex · 2017-04-21T23:04:24Z

src/printer.js

@@ -3024,8 +3029,21 @@ function printJSXElement(path, options, print) {
  let forcedBreak = willBreak(openingLines);

  const jsxWhitespace = options.singleQuote
-    ? ifBreak("{' '}", " ")
-    : ifBreak('{" "}', " ");
+    ? ifBreak(concat([softline, "{' '}", softline]), " ")


the mutations involved later on with this seem pretty bad... but I think you may be on the right track in general. How about something like this:

change this to: const jsxWhitespace = options.singleQuote ? "{' '}" : '{" "}';

where jsxWhitespace is used above, change its usage to ifBreak(concat([jsxWhitespace, softline]), " ") (with the appropriate usage of softline, typically as indicated in the former code).

Thoughts?

rattrayalex · 2017-04-21T23:05:16Z

tests/jsx-split-attrs/__snapshots__/jsfmt.spec.js.snap

@@ -122,8 +122,7 @@ long_open_long_children = (
        colour="blue"
        size="large"
        submitLabel="Sign in with Google"
-      />
-      d
+      />d


This really seems wrong to me... is there any way to not use this algorithm if one of the elements has broken?

If I write <div with="long attributes..." />blah, the "blah" should really be on its own line if the div breaks.

Yep, I agree!

In my first prototype it did have the behaviour of ensuring that the you always had a line break before and after elements that were broken. I must have lost that behaviour in one of my many reworkings of the code.

I'll aim to fix this up, hopefully some time next week.

karl · 2017-04-23T20:46:26Z

I've pushed a few more commits that start to tidy up the code around the fill algorithm and the JSX whitespace declarations.

I've also fixed a couple of other bugs I found while adding new test cases.

rattrayalex

Coming along... I'm still a bit concerned about how well this would translate to similar use-cases, like comments and strings.

Any thoughts?

rattrayalex · 2017-04-24T19:35:28Z

src/printer.js

      if (i === 0) {
-        groups.unshift(child);
+        if (children.length === 1) {


I think it might be a bit more clear if this were pulled out...

if (child === innerJsxWhitespace) { if (children.length === 1) { groups.unshift(solitaryJsxWhitespace); groups.pop(); // remove unnecessary empty group return; } else if (i === 0) { groups.unshift(leadingJsxWhitespace); return; } else if (i === children.length - 1) { groups.push(trailingJsxWhitespace); return; }

I've now made this tweak!

rattrayalex · 2017-04-24T19:38:07Z

src/printer.js

@@ -3154,6 +3174,16 @@ function printJSXElement(path, options, print) {
    }
  });

+  if (children[0] === innerJsxWhitespace) {
+    children[0] = leadingJsxWhitespace;


I really don't like these mutations... why does it have to be done this way?

No reason it needs to be done this way, it was just the quickest way for me to test whether the idea of having different leading/trailing whitespace would work.

Very happy to try some different approaches to this. I wonder if the simplest would be to flatten the groups array. That way we wouldn't need to duplicate the logic of converting inner whitespace to leading/trailing whitespace.

After thinking about this, I believe this whole chunk of code can be removed.

It seems that children is only used in the case where the JSX element is printed on a single line, in which case all the types of JSX whitespaces behave the same, so we don't need to special case leading/trailing/solitary whitespace.

I haven't been able to come up with a failing test case after removing this code, so it seems like a safe change to make.

jlongster · 2017-04-24T20:45:46Z

@rattrayalex I haven't looked at this PR for a while, are you implying that we would rewrite comments and strings? I'm very hesitant to do that. Those are things people can format as they wish, and I think we should intentionally avoid touching them and allow people to do what they want. I feel like it would be too prescriptive/aggressive and too many edge cases to try to rewrite those.

…using fill

karl · 2017-05-07T19:51:40Z

@vjeux I've fixed up the middle two bugs (words incorrectly output one per line, and extra blank lines.

In the process of investigating how we decide to render the , on a new line I found and fixed another issue where we would occasionally output text on the same line as a element that rendered across multiple lines. Now we always have a new line after a child element that is rendered across multiple lines. It turns out doing this simplified a chunk out the JSX rendering!

grydstedt · 2017-05-09T21:40:31Z

@karl Testing out your branch to see if I can get rid of some {' '}. Great work!

I noticed one thing, not sure if it's intended:

becomes

(it adds the {' '}). However, this does not happen if aVariable is replaced by text.

vjeux · 2017-05-09T21:43:06Z

It sounds like {' '}tag in it can be on the same line

karl · 2017-05-09T22:00:30Z

Having the{' '} always be on a line by itself was the easiest way to implement the JSX white space. It works well when separating content that ends up being multi line. I wonder if we could tweak the layout in certain situations.

Do you think this is something that would block the PR?

vjeux · 2017-05-09T22:03:08Z

No, I'm happy to merge with this. Sorry I'm oncall this week and need to find time to run it through again.

vjeux · 2017-05-10T23:13:57Z

So so so sorry for the super long wait, I just ran this on our codebase and almost nothing changed, so I'm happy. Thanks for fixing all the edge cases :)

vjeux · 2017-05-10T23:16:21Z

For the issue with the comma, what about the following heuristic: if there is no space between the text and the jsx tag, then keep it that way?

This is also an issue for (<tag />)

-        (<fbt:param name="Number of profile items to update">
+        (
+        <fbt:param name="Number of profile items to update">
           {itemCount}
-        </fbt:param>)
+        </fbt:param>
+        )

karl · 2017-05-11T07:05:03Z

Thanks for finding the time to merge this @vjeux! And thanks to you and @rattrayalex for patiently taking me through my first major PR, it's been a pleasure 😀

karl · 2017-05-11T08:51:38Z

☝🏻 I've opened a few issues to discuss possible follow on work from this PR.

pkarl · 2017-05-15T19:26:00Z

Awesome!

karl force-pushed the jsx-text-wrap branch 3 times, most recently from ed785a8 to 9444106 Compare April 5, 2017 15:54

rattrayalex reviewed Apr 5, 2017

View reviewed changes

karl changed the title ~~WIP: New primitive to fill a line with as many docs as possible~~ WIP: New primitive to fill a line with as many doc parts as possible Apr 6, 2017

karl force-pushed the jsx-text-wrap branch 2 times, most recently from 5b2376a to ae09ade Compare April 19, 2017 11:53

karl changed the title ~~WIP: New primitive to fill a line with as many doc parts as possible~~ [WIP] New primitive to fill a line with as many doc parts as possible Apr 20, 2017

karl force-pushed the jsx-text-wrap branch from ae09ade to 10c5e9a Compare April 20, 2017 08:22

karl force-pushed the jsx-text-wrap branch 3 times, most recently from ad4ffb5 to e51b623 Compare April 20, 2017 08:48

karl mentioned this pull request Apr 20, 2017

Consider breaking on text in JSX #963

Closed

rattrayalex reviewed Apr 20, 2017

View reviewed changes

rattrayalex reviewed Apr 21, 2017

View reviewed changes

karl force-pushed the jsx-text-wrap branch from 2f22630 to c5e4a1f Compare April 24, 2017 07:47

rattrayalex reviewed Apr 24, 2017

View reviewed changes

karl added 2 commits May 7, 2017 19:16

Make it explicit that we keep multiple consecutive spaces

1858d5e

Add an explanatory comment

9884b3a

karl force-pushed the jsx-text-wrap branch from cce840c to 9884b3a Compare May 7, 2017 18:20

karl added 3 commits May 7, 2017 19:24

Fix broken snapshot in master

50908f6

Ignore existing commands when deciding whether content will fit when …

4b4e365

…using fill

Fix a bug where children would get incorrectly filled onto a line

83397ec

Tidy up JSX whitespace names

b84438e

vjeux mentioned this pull request May 9, 2017

Option to turn off {' '} tags in JSX #1569

Closed

vjeux merged commit 5cda955 into prettier:master May 10, 2017

This was referenced May 11, 2017

Hug JSX text after multi line tags #1581

Closed

Don't always put {" "} on a new line when formatting JSX children #1582

Closed

Recombine split JSX lines #1583

Closed

This was referenced May 22, 2017

Collapses spaces between words in JSX #1661

Closed

Fix fill wrapping when text starts with whitespace #1666

Merged

Unstable JSX formatting #1703

Closed

karl mentioned this pull request Jun 10, 2017

JSX formatting issue with Sublime Text 3 #1789

Closed

karl deleted the jsx-text-wrap branch June 23, 2017 21:09

callmeaponte mentioned this pull request Jul 12, 2017

Any whitespace inserted between lines of JSX text gets collapsed #2463

Closed

lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jan 20, 2019

lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019

New primitive to fill a line with as many doc parts as possible #1120

New primitive to fill a line with as many doc parts as possible #1120

Conversation

karl commented Apr 5, 2017 • edited by vjeux

vjeux commented Apr 5, 2017

karl commented Apr 5, 2017

rattrayalex commented Apr 5, 2017

rattrayalex left a 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

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

karl Apr 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karl commented Apr 5, 2017

rattrayalex commented Apr 6, 2017

jlongster commented Apr 7, 2017

jlongster commented Apr 7, 2017

karl commented Apr 20, 2017

rattrayalex left a 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

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

Choose a reason for hiding this comment

karl commented Apr 20, 2017

rattrayalex commented Apr 20, 2017 • edited

karl commented Apr 21, 2017

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

karl commented Apr 23, 2017

rattrayalex left a 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

Choose a reason for hiding this comment

jlongster commented Apr 24, 2017

karl commented May 7, 2017

grydstedt commented May 9, 2017

vjeux commented May 9, 2017

karl commented May 9, 2017

vjeux commented May 9, 2017

vjeux commented May 10, 2017

vjeux commented May 10, 2017

karl commented May 11, 2017

karl commented May 11, 2017 • edited

pkarl commented May 15, 2017

karl commented Apr 5, 2017 •

edited by vjeux

karl Apr 5, 2017 •

edited

rattrayalex commented Apr 20, 2017 •

edited

karl commented May 11, 2017 •

edited