Allow JSX lines to be recombined by karl · Pull Request #1831 · prettier/prettier

karl · 2017-05-31T15:09:39Z

This PR is an initial investigation into the feasibility to having Prettier recombine JSX lines that have already been split.

Inspired by: #1583

This still has a number of edge cases that need to be handled, but the basics are there.

For example:

const Abc = () => {
  return (
    <div>
      Please state your
      {" "}
      <b>name</b>
      {" "}
      and
      {" "}
      <b>occupation</b>
      {" "}
      for the board of directors.
    </div>
  );
};

Would become:

const Abc = () => {
  return (
    <div>
      Please state your <b>name</b> and <b>occupation</b> for the board of
      directors.
    </div>
  );
}

Allowing lines of JSX to be recombined is a big change from our current approach where we always retain newlines in JSX.

Issue #1583 mentions just recombining lines split with {" "} whereas this PR takes the approach of allowing any lines to be recombined.

I'd be interested in people's opinions on this. Do we think allowing lines to be recombined, as we do with JS, is a good idea for JSX?

P.S. I've based this PR on top of some other PRs I've been working on, only the last commit contains logic for recombining lines.

vjeux · 2017-05-31T16:48:06Z

I'm happy with the direction. The only thing I can think that could go weirdly is people writing paragraphs separated by \n\n, but in practice, they would have to add a

or something around. So I think it's a non-issue in practice.

vjeux · 2017-05-31T17:02:34Z

Btw, i'm going to do a big release on Friday, so if you want it in, would be awesome to ship this today or tomorrow :)

karl · 2017-05-31T17:11:17Z

I'd love to be able to get this change in for Friday, but I'm off on holiday for the next two days so it will need to wait!

That also gives us time to gather more feedback, as I know @rattrayalex had some ideas about recombining lines when I started work on my initial PR for the fill primitive.

vjeux · 2017-05-31T17:17:19Z

Sounds good! I just merged the other one :) Enjoy your vacation!

SimenB · 2017-06-02T17:19:05Z

-        {this.state.bar.qux}
-      </div>
-    );
+    return <div>{this.state.bar.qux}</div>;


I don't like these changes, I found it more readable before

@karl is there a way to prevent these kinds of changes? or are they intentional?

This was intentional, in our codebase at Geckoboard we generally put single expressions in a tag on a single line (if they fit).

I'it's easy to tweak this to never inline if an element contains expressions.

I'll test this out on some open source projects and see which approach gives less changes. Although that doesn't have to be the deciding factor!

I ran both options (break if an element contains an expression vs. break if an element contains more than one expression) over the Geckoboard codebase to see how they compared.

I noticed that always breaking on expressions caused some less than ideal layout in a few cases:

You currently have {dashboardStr} and {userStr}

becomes:

You currently have{' '} {dashboardStr} {' '} and{' '} {userStr} 

I'm not sure yet how we could avoid the above output will still aiming to break elements that contain a single expression. Any ideas greatly appreciated!

I've run both variations over the following open source projects:

https://github.com/andrewngu/sound-redux

https://github.com/WebbyLab/itsquiz-wall

https://github.com/echenley/react-news/

Allowing elements with a single expression to be inlined generally resulted in less changes than always expanding.

SimenB · 2017-06-02T17:20:12Z

-        <HelloLocal />
-      </div>
-    );
+    return <div><Hello /><HelloLocal /></div>;


I really don't like this.

SimenB · 2017-06-02T17:21:05Z

-    Welcome to the <strong>Universal React Starter-kyt</strong>.
-    This starter kyt should serve as the base for an advanced,
-    server-rendered React app.
+    Welcome to the <strong>Universal React Starter-kyt</strong>. This starter


stuff like this is perfect though, IMO 🙂

SimenB · 2017-06-02T17:22:11Z

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


here it would be better to have the {" "} after foo, not before . Right?

I'm interested in your thoughts on this. Based on this issue (#1582) I went with having {" "} at the start of lines. But I don't have strong feeling either way!

I don't like it because messes up the alignment of opening and closing tags.

I have a PR that switches {" "} positioning around so they now appear at the end of lines rather than the beginning. I'd be interested in your thoughts.

#1964

karl · 2017-06-02T19:31:20Z

Thanks for the feedback @SimenB! Great to know what bits people like and don't like.

I'm hoping to refine this proof of concept over the next few weeks, which should give us lots of time to gather feedback and work out if this is a sensible change to pursue.

karl · 2017-06-05T11:45:09Z

I've pushed an updated version of this PR with a new heuristic which renders the opening and closing times of their own lines if the JSX contains any tag elements.

This improves some of the formatting issues pointed out by @SimenB.

For example:

x = (
  <div>
    <Hello />
    <HelloLocal />
  </div>
);

Now becomes:

x = (
  <div>
    <Hello /><HelloLocal />
  </div>
);

Whereas in the previous version of this PR it would output:

x = <div><Hello /><HelloLocal /></div>;

JSX containing just text or expressions (e.g. {foo}) will still render on a
single line if there is enough space.

For example:

x = <div>I like monkeys</div>;

y = <div>{"An expression " + foo}</div>;

The output might not be good enough yet, but it is starting to look more sane :)

karl · 2017-06-05T11:47:53Z

+            </div>
+          </div>
+        </div>
+      </div>


One consequence of the new heuristic is that deeply nested tags now expand to take up more lines. This might be nice as it makes it very explicit that you have a high level of nesting.

If we want to avoid this we could change the heuristic to only expand elements if they have at least two JSX tags.

I like this 🙂 As you say, it highlights deep nesting, which i fine

FWIW, the original behavior was very much intentional.

The deep nesting is plenty visible in the inline case, and if it needs to be done, why make it even more annoying?

Especially with Styled Components and the like taking off, I don't think we'd want to effectively discourage things like this:

render() { return ( <MyList> {this.props.items.map(item => ( <MyListItem><Text><MyDomainSpecific>{item}</MyDomainSpecific></Text></MyListItem> ))} </MyList> ) }

SimenB · 2017-06-05T11:55:14Z

-  <div style={styles} key="something">
-    Keep the wrapping parens.
-  </div>
+  <div style={styles} key="something">Keep the wrapping parens.</div>


I still think this was more readable before, but I have no idea which heuristic would make it better...

SimenB · 2017-06-05T11:56:01Z

It's definitely better, IMHO. Really looking forward to seeing where this lands!

karl · 2017-06-05T12:20:41Z

I've pushed up another tweak to always make JSX elements with attributes and children multiline.

This makes the following always multiline:

x = (
  <div attr="something">
     Some text
  </div>
);

I think this improves the readability in general, at the cost of occasionally making something multiline that doesn't really need to to be.

Again, we could tweak this to only apply if there are multiple attributes if we want to reduce the number of unnecessary breaks onto multiple lines.

karl · 2017-06-05T14:35:43Z

+    <h2>First</h2><p>The first paragraph.</p><h2>Second</h2>
+    <p>The second paragraph.</p>
  </div>
 );


The output here is pretty terrible!

rattrayalex · 2017-06-05T18:06:55Z

I'm fairly worried that this is getting too proscriptive & special-case-y...

For example,

x = (
  <div>
    <Hello />
    <HelloLocal />
  </div>
);

could very easily be the best formatting in some situations, better than

x = (
  <div>
    <Hello /><HelloLocal />
  </div>
);

The former keeps the components more atomic and highlights that they are separate siblings.

I might propose that this change targets only "paragraphs of text", not "series of elements".

I know that sounds difficult to determine, especially when elements like  and <a> are mixed in. An algorithm along the lines of "starts and ends with text, where the total length of the text is greater than the total length of the elements" could work but sounds messy. Simpler might be to apply it only to elements who have at least one text child (not counting whitespace).

Just spitballing / 2¢

karl · 2017-06-05T18:27:05Z

And a very good 2¢ they are @rattrayalex! I was thinking many of the same thoughts myself.

I think a heuristic of requiring at least one text element (that isn't just whitespace) would be a good heuristic to try. It would fix up the terrible formatting in the headers_and_paragraphs example.

I agree that this could end up with too many special cases, and if that does happen I'm happy to write it off as an interesting experiment.

vjeux · 2017-06-05T19:55:02Z

I might propose that this change targets only "paragraphs of text", not "series of elements".

Sounds like a really good heuristic!

karl · 2017-06-07T19:27:18Z

I've updated the PR with the heuristic we discussed above.

If the JSX element contains meaningful text (something other than whitespace) then we use fill to layout the children. If it only contains tags and expressions (no meaningful text) then we group to layout the children.

no_text_one_tag_per_line =
  <div>
    <first /><second />
  </div>

with_text_fill_line =
  <div>
    Text <first /><second />
  </div>

becomes

no_text_one_tag_per_line = (
  <div>
    <first />
    <second />
  </div>
);

with_text_fill_line = (
  <div>
    Text <first /><second />
  </div>
);

karl · 2017-06-07T22:34:10Z

I've run this PR over the full Geckoboard codebase and make a couple of tweaks based on the result.

  tags are now special cased and are always followed by a line break.

<div>A<br />B<br />C</div>

becomes

<div>
  A<br />
  B<br />
  C
</div>

I also tweaked the attributes heuristic to only force a break when a tag has 2 or more attributes. It seemed like a nice balance of allowing some inlining while breaking when complexity increased.

karl · 2017-06-22T09:09:29Z

With the example below:

       <div>
-        Updated every 1 second: "
-        <RelativeDate date={new Date()} delay={1000} />
+        Updated every 1 second: "<RelativeDate date={new Date()} delay={1000} />
         "
       </div>

I believe the " is appearing on a new line because the previous line reaches the print width exactly. If it is shorter or longer by 1 character you get different output with the " next to the tag.

vjeux · 2017-06-22T15:41:58Z

Can I hit the merge button? :)

karl · 2017-06-22T15:44:06Z

I'm just testing a bug fix, I'll give you a ping when I'm happy with it!

karl · 2017-06-22T16:11:21Z

@vjeux I'm all done with my testing, very happy to have this merged in!

Once it is merged I'll create new issues to track the follow on work this PR has generated (e.g. better output for the example you posted above, better output of single expressions as discussed with @rattrayalex, and adding a flag to toggle FB translation pipeline compatibility).

vjeux · 2017-06-22T16:20:10Z

🎉

SimenB · 2017-06-22T16:24:33Z

Woo! 🎉
ETA for release 👼?

vjeux · 2017-06-22T16:27:19Z

Hopefully tomorrow.

karl · 2017-06-22T16:47:50Z

I've created a number of follow on issues based on the discussion in the PR, if I've missed anything please feel free to create a new issue and ping me!

rattrayalex · 2017-06-23T17:39:13Z

Woohoo, awesome work @karl !!! Very exciting!

The handful of tricky cases around expressions etc definitely make sense as follow-ons, I'll try to take a look and be helpful there if I can, though you seem to have this stuff on lock 😄

josephfrazier mentioned this pull request Jun 1, 2017

Add cursorOffset option for cursor translation #1637

Merged

SimenB reviewed Jun 2, 2017

View reviewed changes

This was referenced Jun 3, 2017

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

Closed

Position JSX whitespace ({" "}) at the end of lines #1964

Merged

karl force-pushed the jsx-recombine-lines branch 2 times, most recently from 84e21c0 to c6c686c Compare June 5, 2017 11:31

karl commented Jun 5, 2017

View reviewed changes

SimenB reviewed Jun 5, 2017

View reviewed changes

karl commented Jun 5, 2017

View reviewed changes

karl mentioned this pull request Jun 6, 2017

Doesn't inline JSX if less than 80 chars (and contains newline) #2001

Closed

karl force-pushed the jsx-recombine-lines branch 3 times, most recently from 86c1086 to 2bccfc0 Compare June 7, 2017 19:26

karl force-pushed the jsx-recombine-lines branch from 1682981 to 4aa9637 Compare June 7, 2017 22:37

karl force-pushed the jsx-recombine-lines branch from e019ae3 to 16f8b19 Compare June 22, 2017 14:44

karl force-pushed the jsx-recombine-lines branch from a3524bc to d241c9b Compare June 22, 2017 15:44

karl added 4 commits June 22, 2017 16:45

Allow JSX lines to be recombined

71c8b94

Add test case for breaking expression

b18d157

Fix issue where JSX whitespace could be dropped

f8796e4

Newlines were incorrectly being preserved when JSX contained text

40b3e35

karl force-pushed the jsx-recombine-lines branch from d241c9b to 40b3e35 Compare June 22, 2017 15:45

vjeux merged commit 0cc0ebc into prettier:master Jun 22, 2017

karl deleted the jsx-recombine-lines branch June 26, 2017 19:43

karl mentioned this pull request Jun 27, 2017

Recombine split JSX lines #1583

Closed

renovate Bot mentioned this pull request Jun 27, 2017

Update dependency prettier to version 1.5.2 renovatebot/renovate#365

Merged

karl mentioned this pull request Jun 28, 2017

JSX text content moving to separate line #2324

Closed

SimenB mentioned this pull request Jun 28, 2017

JSX whitespace moved to same line as tag #2325

Closed

azz mentioned this pull request Jul 15, 2017

Print width ignored for JSX elements with multiple attributes #2477

Closed

duailibe mentioned this pull request May 23, 2018

Add line break before forced space to stay within the print width #4527

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

Uh oh!

Conversation

karl commented May 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vjeux commented May 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vjeux commented May 31, 2017

Uh oh!

karl commented May 31, 2017

Uh oh!

vjeux commented May 31, 2017

Uh oh!

SimenB Jun 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karl Jun 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SimenB Jun 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karl commented Jun 2, 2017

Uh oh!

karl commented Jun 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rattrayalex Jun 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SimenB commented Jun 5, 2017

Uh oh!

karl commented Jun 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rattrayalex commented Jun 5, 2017

Uh oh!

karl commented Jun 5, 2017

Uh oh!

vjeux commented Jun 5, 2017

Uh oh!

karl commented Jun 7, 2017

Uh oh!

karl commented Jun 7, 2017

Uh oh!

karl commented Jun 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vjeux commented Jun 22, 2017

Uh oh!

karl commented Jun 22, 2017

karl commented May 31, 2017 •

edited

Loading

vjeux commented May 31, 2017 •

edited

Loading

SimenB Jun 2, 2017 •

edited

Loading

karl Jun 16, 2017 •

edited

Loading

SimenB Jun 2, 2017 •

edited

Loading

rattrayalex Jun 5, 2017 •

edited

Loading

karl commented Jun 22, 2017 •

edited

Loading