Several JSX fixes (addresses #73) #123

rattrayalex · 2017-01-11T18:36:39Z

I wasn't sure where to add the new test cases, so I just threw them as separate files in the prettier dir. Happy to move/rename etc as desired.

jmorrell · 2017-01-11T18:53:21Z

tests/prettier/jsx_long_name_closed.js

@@ -0,0 +1 @@
+<BaseForm url="/auth/google" method="GET" colour="blue" size="large" submitLabel="Sign in with Google"/>


Would you want to move these under a new tests/jsx directory?

The tests situation is a little weird, but I'm hesitant to touch much outside of the prettier directory because all of those tests are Flow's test suite. In the future we should be able to remove our version and copy in the latest Flow tests.

vjeux · 2017-01-11T18:59:47Z

tests/prettier/__snapshots__/jsfmt.spec.js.snap

+  colour=\"blue\"
+  size=\"large\"
+  submitLabel=\"Sign in with Google\"
+>


Most conventions I've seen is to put the > in the same line as the last attribute. This is likely going to be a bit trickier to implement but I think that it's important.

rattrayalex · 2017-01-11T19:03:13Z

Oh, that's actually easier. I did it this way on purpose as it conforms with the Airbnb style guide, which is what I use. Happy to put it on the trailing line if that's preferred. Might warrant some discussion though, perhaps in #73

…

On Wed, Jan 11, 2017, 10:59 Christopher Chedeau ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In tests/prettier/__snapshots__/jsfmt.spec.js.snap <#123 (review)> : > +/>; +" +`; + +exports[`test jsx_long_name_open.js 1`] = ` +"<BaseForm url=\"/auth/google\" method=\"GET\" colour=\"blue\" size=\"large\" submitLabel=\"Sign in with Google\"> + hello +</BaseForm> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +<BaseForm + url=\"/auth/google\" + method=\"GET\" + colour=\"blue\" + size=\"large\" + submitLabel=\"Sign in with Google\" +> Most conventions I've seen is to put the > in the same line as the last attribute. This is likely going to be a bit trickier to implement but I think that it's important. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#123 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAq_LtaaC3HFNvhEHEDH-GibQId5_HYNks5rRSakgaJpZM4Lg65c> .

rattrayalex · 2017-01-11T19:05:08Z

The advantage of the Airbnb style is atomicity - similar to the argument in favor of trailing commas. Better git diff's etc.

…

On Wed, Jan 11, 2017, 11:02 Alex Rattray ***@***.***> wrote: Oh, that's actually easier. I did it this way on purpose as it conforms with the Airbnb style guide, which is what I use. Happy to put it on the trailing line if that's preferred. Might warrant some discussion though, perhaps in #73 On Wed, Jan 11, 2017, 10:59 Christopher Chedeau ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In tests/prettier/__snapshots__/jsfmt.spec.js.snap <#123 (review)> : > +/>; +" +`; + +exports[`test jsx_long_name_open.js 1`] = ` +"<BaseForm url=\"/auth/google\" method=\"GET\" colour=\"blue\" size=\"large\" submitLabel=\"Sign in with Google\"> + hello +</BaseForm> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +<BaseForm + url=\"/auth/google\" + method=\"GET\" + colour=\"blue\" + size=\"large\" + submitLabel=\"Sign in with Google\" +> Most conventions I've seen is to put the > in the same line as the last attribute. This is likely going to be a bit trickier to implement but I think that it's important. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#123 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAq_LtaaC3HFNvhEHEDH-GibQId5_HYNks5rRSakgaJpZM4Lg65c> .

vjeux · 2017-01-11T19:10:27Z

Yeah, probably warrants some discussions. I didn't know that Airbnb recommended this.

On the React docs we use the one I suggested and across our codebase at Facebook as well.
https://facebook.github.io/react/docs/optimizing-performance.html#examples

gaearon · 2017-01-11T20:14:37Z

TBH I don't understand why we do this at Facebook. Putting it on the new line means less friction when adding/removing attributes, and is more diff-friendly. Also easier to read when there is a bunch of attributes and children.

rattrayalex · 2017-01-11T20:18:57Z

Airbnb doesn't offer any justification, nor is there any in the eslint rule docs.

Would it be easy to find out why FB does this? Would it be possible to change FB's internal guide (so that prettier could be adopted within FB even with this change)?

jlongster · 2017-01-11T20:22:10Z

I agree, see this example:

      <button
        color={this.props.color}
        onClick={() => this.setState(state => ({count: state.count + 1}))}>
        Count: {this.state.count}
      </button>

You don't really see where the opening tag ends. It looks like Count is an attribute. However, if Facebook is interested in using this, I want to make sure we talk about this more.

vjeux · 2017-01-11T20:49:25Z

I don't think that there's a good logical argument for > on the last line. This dates back to XHP days.

Now, this is a pretty visible change and there's already going to be a lot of friction for integrating prettier inside of the Facebook codebase so I would love to avoid a big debate on this and if we could get either use this form or get an option for it at the very least.

jlongster · 2017-01-11T21:07:01Z

It'd be an easy option to make, but need to decide if we should have that option or not. If that's the only thing that would stop Facebook from adopting this it's tempting to make it an option, but I also want to be fair to other users who want other options.

@vjeux What do you think about putting > on a new line by default, with the caveat that you will talk with your coworkers and see if this is the only thing stopping them from using it and we can follow-up with possibly making it an option?

vjeux · 2017-01-11T21:09:08Z

Let's do that. There is a bunch more blockers until we can actually use it. When that list is down to something where we can actually test it, we'll see how people react.

rattrayalex · 2017-01-11T21:30:34Z

Awesome! Is this okay to merge or is there anything I can improve here?

…

On Wed, Jan 11, 2017, 13:09 Christopher Chedeau ***@***.***> wrote: Let's do that. There is a bunch more blockers until we can actually use it. When that list is down to something where we can actually test it, we'll see how people react. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#123 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAq_LjVxWpZLlfrNkXPd7tVn6cPPy43Tks5rRUT1gaJpZM4Lg65c> .

rattrayalex · 2017-01-11T23:15:38Z

I added a second commit to this PR fixing parens. Let me know if I should split that out to a separate PR.

vjeux · 2017-01-11T23:26:19Z

tests/prettier/__snapshots__/jsfmt.spec.js.snap

+  submitLabel=\"Sign in with Google\"
+/>;
+
+long_open = <BaseForm


I'm curious, since you add () around arrow functions, should you also do it here for consistency?

Possibly...

In this case, I'm only using variables so that one file can contain multiple JSX statements, which otherwise can't be siblings.

That would seem to conform to AirBnB's guide so sure why not!

Hmm... I should probably refactor this (and any others like the corresponding code in ReturnStatement) within JSXElement itself... might be tricky...

vjeux · 2017-01-11T23:33:07Z

I really like your approach of going through the Airbnb style-guide as a reference when in doubt :)

rattrayalex · 2017-01-12T02:13:17Z

Also added a fix for #73 (comment) , though using atrr={"\""} instead of attr=""", as recommended in the react docs.

jlongster · 2017-01-12T14:21:00Z

@rattrayalex This will conflict with #126, any chance you could remove the commits that do the escaping? I'm not sure I want to force the {"\""} but rather just keep the original code. (I'm not sure if you will convert attr=""" to atrr={"\""} automatically, haven't looked too closely at this code)

jlongster

See above

rattrayalex · 2017-01-12T14:24:07Z

Yep can do!

jlongster · 2017-01-12T14:26:39Z

src/printer.js

+        indent(options.tabWidth, concat([ line, path.call(print, "body") ])),
+      ]));
+    }
+    return conditionalGroup(groups);


I would prefer avoiding conditionalGroup in this special case which makes this code path a little clearer:

const body = path.call(print, "body"); const collapsed = concat([ concat(parts), " ", body ]); if (n.body.type === "JSXElement") { return group(collapsed); } return conditionalGroup([ collapsed, groups.push(concat([ concat(parts), indent(options.tabWidth, concat([ line, body ])), ])); ])

Also note that I lifted path.call(print, "body") out which is very important. I wrote that code so it's my fault, but when using conditionalGroup you should never print the same node twice because it makes the intermediate representation explode in size (we want to share sub-trees).

awesome, will do!

hah, I'd originally wrote it with body separated as you have now, and changed it back. Good to hear of the tradeoffs

jlongster · 2017-01-12T14:54:25Z

src/printer.js

+    ")",
+  ]));
+
+  return conditionalGroup(groups);


I don't think you need conditionalGroup here. Use conditionalGroup only as a last resort :) (I plan on writing more documentation about all this)

conditionalGroup is only needed when you really need manual control over how things are displayed when they don't fit. The majority of use cases fits into these 2 categories though:

You want to break something across lines and indent if it doesn't fit (this assumes nothing inside of it has "hard" lines, meaning all contents try to fit on the same line)

You want to break something across lines if any of its children are broken across lines (they have "hard" lines)

group solves #1 easily: use indent and line and the printer, when the group doesn't fit, will output newlines where line was used and it will be indented an extra level.

I made multilineGroup for #2 because it's a very common use case. It will see if any of its contents have hard lines, and if they do, the group will break. Hard lines are lines that emit a newline no matter what. The reason we don't have to worry about normal breaking is because this group is always larger than it's contents -- if anything inside of it naturally breaks, this group will already have broken as well.

There are some rare cases where you want to let something inside of you break, but not break the group, and control indentation rules in each specific case. That's what conditionalGroup is for, but we should use it rarely as it has a performance cost.

There's one last character in this cast: ifBreak. This is a very simple construct that does what it says: if the group break, include this content. You can say ifBreak(",") and it will only include the comma if the group has broken. The solves some very simple use cases.

Will all that said, I think this should work:

multilineGroup(concat([ ifBreak("("), indent(options.tabWidth, concat([ softline, elem ])), softline, ifBreak(")"), ]))

That's extremely helpful! Also sounds like a great cleanup. Thanks!

vjeux · 2017-01-12T16:01:39Z

tests/prettier/__snapshots__/jsfmt.spec.js.snap

+
+const render5 = ({ styles }) => <div>Keep it on one line.</div>;
+
+const notJSX = (aaaaaaaaaaaaaaaaa, bbbbbbbbbbb) =>


This is glorious

vjeux · 2017-01-12T16:01:50Z

deleteme.txt

@@ -0,0 +1 @@
+blah


I think this is the only thing left to do, right? Once you remove this, is this ready to be merged? Also check the test failures (slow to respond right now, will focus more later today)

jlongster · 2017-01-12T16:05:01Z

@rattrayalex When you rebase locally, you should be able to just git push -f ?

rattrayalex · 2017-01-12T17:33:05Z

Sorry, I hadn't run jest -u after the most recent change, and found another edge case that refactored code didn't handle as well... the underlying issue was a separate problem, which I am now fixing, and hope to add to this PR if that's ok.

(children need to be broken up if the open element is multiline)

jlongster · 2017-01-12T18:38:39Z

Sure, feel free to add any small fixes to this PR. I wanted to do another pass at JSX support and improve many things at once, and this is basically that! Thanks.

rattrayalex · 2017-01-12T20:33:34Z

Awesome! have a few more small fixes in the pipeline, should be done by EOD or tomorrow.

rattrayalex · 2017-01-12T20:47:13Z

src/printer.js

+    const firstChild = children[0];
+    const lastChild = util.getLast(children);
+    const beginBreak = firstChild && firstChild.type === "line";
+    const endBreak = lastChild && lastChild.type === "line";


@jlongster my beginBreak / endBreak logic here felt a bit awkward – I'm adding a softline unless there's already a line. Is there a cleaner way to do that?

@rattrayalex Sorry I didn't review this until now.

I'm not entirely sure of the logic you're going for here, but is util.newlineExistsBefore helpful? If you're trying to emulate the breaking behavior in the original source, that and util.newlineExistsAfter allows you to peek into the original source and see if a node is on a newline or not. Here's how you use it:

util.newlineExistsBefore(text, util.locStart(node))

What this does is it scans backward until it hits either a newline or a non-whitespace character. If it hits a newline, it reports true, otherwise it's false.

That's definitely helpful to know about, though in this case I think the lines could be inserted by prettier, not just from the original source. So I think it wouldn't be adequate in this case.

But it would be very helpful when deciding whether {' '} needs to be inserted, if that's a route we go.

rattrayalex · 2017-01-12T20:48:52Z

tests/prettier/__snapshots__/jsfmt.spec.js.snap

+        attr4
+      >
+        ddd d dd d d dddd dddd <strong>hello</strong>
+      </div><strong>hello</strong>


fwiw, I'm trying to fix this now (bump siblings to own line)

I couldn't figure out a clean solution to this, so leaving for now.

I don't think we can do that. We would be adding significant whitespace, which changes the program. That's a limitation because of how JSX works; we aren't as free to move things around as other things.

As it turns out, adding newlines between JSX nodes doesn't add whitespace between the nodes to the compiled HTML. So that would be a safe change to make in this case.

But it would have to be a newline & a bunch of spaces for indentation. How could that not add whitespace?

JSX isn't HTML, the elements compile to function calls:

React.createElement('div', {}, [ React.createElement('span', {}, ['hello world']), React.createElement('strong', {}, ['I am a sibling']) ])

^^ there's no whitespace between the <span /> and the <strong /> above. That would render to:

<div><span>hello world</span><strong>I am a sibling</strong></div>

rattrayalex · 2017-01-12T21:56:00Z

tests/prettier/__snapshots__/jsfmt.spec.js.snap

+const render7 = () => (
+  <div>
+    <span />
+    <span>Break each elem onto its own line.</span>


I'm pretty iffy on this. There don't seem to be any eslint rules about preventing multiple JSX elements on one line...

I think I'll cut this, even if it means having this on the same line:

</div><span>should really be next line</span>

though if anyone can figure out a solution to that case, I'm all ears

…rt of #73)

rattrayalex · 2017-01-12T23:53:59Z

Hmm, the other challenges I had hoped to tackle look a bit too challenging for my current knowledge of the tools at hand. (Actually, I suspect they may not be possible with the current tools at hand).

Specifically:

inserting {' '} only on artificial line breaks, and otherwise a literal , doesn't seem possible. ifBreak("{' '}") will add the element because of breaks higher up the tree (not just immediately before/after the element) and enclosing jsx children in a group or multilineGroup breaks the parent jsx element multilineGroup.
as mentioned in General JSX Improvements #73 (comment) , dedenting the JSXExpressionContainer's closing }, possibly along with other punctuation, isn't something I was able to figure out how to do.
as mentioned above, breaking </div><strong>hello</strong> after </div> is beyond me for now.

Would appreciate guidance, though for now my preference would be to merge this PR. Perhaps more skilled hands than mine could take up the remainder of this work thereafter.

jlongster · 2017-01-13T01:32:48Z

Thanks a lot for working on this. It's definitely fine to do other things as followup. I'll look at this soon.

vjeux · 2017-01-13T02:33:35Z

FYI, we recently rewrote the JSX documentation to explain how whitespace works:

JSX removes whitespace at the beginning and ending of a line. It also removes blank lines. New lines adjacent to tags are removed; new lines that occur in the middle of string literals are condensed into a single space. So these all render to the same thing:

<div>Hello World</div>

<div>
  Hello World
</div>

<div>
  Hello
  World
</div>

<div>

  Hello World
</div>

https://facebook.github.io/react/docs/jsx-in-depth.html#children-in-jsx

jlongster · 2017-01-13T03:36:30Z

@vjeux @rattrayalex oh, shows how much I know about JSX 😆 I'm just now starting to use it, not that I can consistently format it.

I'm not sure we want to be aggressive about bumping tags to newlines. JSX seems like something that the programmer lays out specifically, so we should be more lenient about respecting JSX. There was someone else talking about not doing this specifically (can't remember where) so that if you had a big paragraph you could have <strong> tags inside it and it would work fine.

jlongster · 2017-01-13T18:07:26Z

@rattrayalex Can you look at my one comment and see if those utility functions are what you're after?

Once we get that comment resolved, I will merge this. Thanks so much for all the hard work!

jlongster · 2017-01-13T19:22:29Z

Alright this has been sitting for too long. I'm eager to get it in and make a release, so I'll go ahead and merge it. Thanks for the excellent work @rattrayalex. This will make prettier usuable for JSX folks!

I'm going to follow-up with the comments I made and possibly make some PRs myself. But if everything mostly checks out, I'll push a new release.

jmorrell reviewed Jan 11, 2017

View reviewed changes

rattrayalex mentioned this pull request Jan 11, 2017

General JSX Improvements #73

Closed

11 tasks

vjeux reviewed Jan 11, 2017

View reviewed changes

rattrayalex force-pushed the master branch from fbc0969 to 1601500 Compare January 11, 2017 23:15

vjeux reviewed Jan 11, 2017

View reviewed changes

rattrayalex force-pushed the master branch 3 times, most recently from b6c3b60 to 8f652af Compare January 12, 2017 02:10

rattrayalex changed the title ~~Break JSXOpeningElement between attributes (fixes #15)~~ Several JSX fixes (addresses #73) Jan 12, 2017

jlongster requested changes Jan 12, 2017

View reviewed changes

rattrayalex force-pushed the master branch 2 times, most recently from e033326 to bc08eb4 Compare January 12, 2017 15:46

rattrayalex closed this Jan 12, 2017

rattrayalex force-pushed the master branch from bc08eb4 to 0eda444 Compare January 12, 2017 15:47

vjeux reviewed Jan 12, 2017

View reviewed changes

rattrayalex force-pushed the master branch from 1b3a7b8 to 8e92db8 Compare January 12, 2017 17:32

rattrayalex commented Jan 12, 2017

View reviewed changes

rattrayalex force-pushed the master branch from aa45c6c to 4c0f59f Compare January 12, 2017 21:50

rattrayalex commented Jan 12, 2017

View reviewed changes

rattrayalex force-pushed the master branch from 4c0f59f to fd1cab3 Compare January 12, 2017 22:35

rattrayalex added 3 commits January 12, 2017 14:37

Wrap Stateless JSX Arrow Functions and Assignment in Parens (fixes pa…

22aef7c

…rt of #73)

Break JSXOpeningElement between attributes (fixes #15)

85a07d4

JSX maintains spaces that matter (fixes #30 and thus part of #73)

2552da5

rattrayalex force-pushed the master branch from fd1cab3 to d203807 Compare January 12, 2017 22:37

rattrayalex added 2 commits January 12, 2017 14:48

Multiline JSX opening tag breaks children out too (for #73)

5075fa6

Add regression tests for long JSX Expression contents

eccf3d1

rattrayalex force-pushed the master branch from d203807 to eccf3d1 Compare January 12, 2017 22:48

bsr203 mentioned this pull request Jan 13, 2017

not using all of eslint conf kentcdodds/prettier-eslint-atom#1

Closed

jlongster approved these changes Jan 13, 2017

View reviewed changes

jlongster merged commit 47102bc into prettier:master Jan 13, 2017

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

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

		@@ -0,0 +1 @@
		<BaseForm url="/auth/google" method="GET" colour="blue" size="large" submitLabel="Sign in with Google"/>


		const render5 = ({ styles }) => <div>Keep it on one line.</div>;

		const notJSX = (aaaaaaaaaaaaaaaaa, bbbbbbbbbbb) =>

		@@ -0,0 +1 @@
		blah

Several JSX fixes (addresses #73) #123

Several JSX fixes (addresses #73) #123

Conversation

rattrayalex commented Jan 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rattrayalex commented Jan 11, 2017 via email

rattrayalex commented Jan 11, 2017 via email

vjeux commented Jan 11, 2017

gaearon commented Jan 11, 2017 • edited

rattrayalex commented Jan 11, 2017

jlongster commented Jan 11, 2017

vjeux commented Jan 11, 2017

jlongster commented Jan 11, 2017

vjeux commented Jan 11, 2017

rattrayalex commented Jan 11, 2017 via email

rattrayalex commented Jan 11, 2017

Choose a reason for hiding this comment

rattrayalex Jan 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vjeux commented Jan 11, 2017 • edited

rattrayalex commented Jan 12, 2017

jlongster commented Jan 12, 2017

jlongster left a comment

Choose a reason for hiding this comment

rattrayalex commented Jan 12, 2017 • edited

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

jlongster commented Jan 12, 2017

rattrayalex commented Jan 12, 2017 • edited

jlongster commented Jan 12, 2017

rattrayalex commented Jan 12, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rattrayalex Jan 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rattrayalex Jan 12, 2017 • edited

Choose a reason for hiding this comment

rattrayalex commented Jan 12, 2017

jlongster commented Jan 13, 2017

vjeux commented Jan 13, 2017

jlongster commented Jan 13, 2017

jlongster commented Jan 13, 2017

jlongster commented Jan 13, 2017

gaearon commented Jan 11, 2017 •

edited

rattrayalex Jan 11, 2017 •

edited

vjeux commented Jan 11, 2017 •

edited

rattrayalex commented Jan 12, 2017 •

edited

rattrayalex commented Jan 12, 2017 •

edited

rattrayalex Jan 13, 2017 •

edited

rattrayalex Jan 12, 2017 •

edited