New rule: jsx-one-expression-per-line

[TSMMark]

From issue #1491.

Tested against a small react-native app with 50-100 components and it works great.

[ljharb]

Thanks, this seems pretty solid.

I gather that you're choosing an indent of zero, so that the indent rule can correct it properly?

[jseminck]

There are only tests for components. Should it also warn on <div><span>Hello World</span></div> ?

[TSMMark]

I gather that you're choosing an indent of zero, so that the indent rule can correct it properly?

That's right.

There are only tests for components. Should it also warn on <div><span>Hello World</span></div> ?

Not sure why that would make a difference, but I added a case for ya.

[ljharb]

If the lack of whitespace before "foo" is important, then it's just as important after "foo" - meaning, this example can't be autofixed.

However,

<div>
  <span /> foo <input />
</div>

definitely should become:

<div>
  <span />
  foo
  <input />
</div>

[TSMMark]

@ljharb I believe white space in jsx is differnt from HTML and in your example the two jsx snippets are actually rendered differently. foo in the first snippet will be rendered to the DOM with a whitespace on either side, while foo in the second snippet will have no whitespace.

However, even if I'm correct the rule still does need to be patched. Currently, the autofix does change the rendered whitespace under certain conditions, which is obviously unacceptable.

[TSMMark]

Using curlies and transforming into a string literal with spacing on either side seems the most accurate. Is this acceptible?

<div>
  <span />
  {' foo '}
  <input />
</div>

[ljharb]

Sure, that'd be great!

As long as the autofix is not changing the resulting whitespace that's rendered, we're good.

[TSMMark]

Hey @ljharb I spec'd out what I think is the best approach for handling white space. Basically, it comes down to not handling it and letting the user fix each case on their own. ptal if you could: 799dfa6

If y'all agree I'll implement this behavior in the next couple days

[ljharb]

@TSMMark i think we'd need to either add an option, or have this be the default, so that the things that aren't autofixable aren't warned.

[TSMMark]

For the record, I am attempting to handle all autofix cases and it is proving rather difficult.

Edit: Just discovered isSpaceBetweenTokens and it looks like this might be exactly what I need to make this work.

[TSMMark]

@ljharb Well, I had to throw everything out and start over, but now we can handle all cases! 😌

How's it look?

[ljharb]

Looking much better :-)

[ljharb]

this can cause issues due to rules that strip trailing spaces; i think this needs to remain as foo {"bar"}, untouched.

[TSMMark]

foo and {"bar"} are 2 different expressions and I don't want them on the same line. JSX parser ignores trailing spaces such as these, however, I'll see if I can kill them, as per #1497 (comment)

[ljharb]

right - but the jsx parser doesn't ignore the space in between foo {"bar" } - so to make it the same, with an autofix, you'd need:

foo
{' '}
{'bar'}

[TSMMark]

Yes, if you look on the next line 117 you will see {\' \'}

[TSMMark]

It results in the same output after JSX parsing. I am following the guidelines:

Since all rules are run again after the initial round of fixes is applied, it’s not necessary for a rule to check whether the code style of a fix will cause errors to be reported by another rule.

https://eslint.org/docs/developer-guide/working-with-rules#applying-fixes

The trailing space in the output foo is inconsequential. Its only impact is offending no-trailing-spaces, which, as stated in the guide, is not our concern here.

[ljharb]

this trailing space should be removed, if the space on the next line is being explicitly preserved?

[TSMMark]

JSX parser already ignores trailing spaces, and no-trailing-spaces will chop them off anyway... but let me see if I can get rid of them. My thought process was the JSX output is identical so I would not need to handle them

[ljharb]

i agree that no-trailing-spaces could do the cleanup, but it seems easy enough to .trim() any newly autofixed lines :-)

[TSMMark]

See latest commit, best I can do right now. As you said, this does trim the newly autofixed nodes; however, it does not handle cases where the 'Literal' node is the first node on the line because it does not need to be placed on a new line, requiring no report/fix for that node.

Example:

https://github.com/Vydia/eslint-plugin-react/blob/1344f1dad2478d15a378d681128206bf969c733b/tests/lib/rules/jsx-one-expression-per-line.js#L714

[ljharb]

also here?