[Fix] jsx-indent with tabs #1058

kentcdodds · 2017-02-01T17:38:23Z

This fixes the --fix for jsx-indent. Specifically my issue was with tabs (Fixes #1057), but this fixes the same issues with spaces as well.

Highlevel important bits:

Adds fix for when the ending of a tag (>) is on its own line. This will now lint and fix properly.
Adds fix for linting and fixing Literal strings inside JSX
Adds configuration option called indentLogicalExpressions which will allow for indenting the right side of logical expressions (documented)
Adds fix for linting and fixing the indentation of the closing tag.

kentcdodds

I think I'm ready for a solid review on this PR. I have verified that these changes fix the repo referenced in #1057

kentcdodds · 2017-02-01T20:09:22Z

lib/rules/jsx-indent.js


-      if (loc) {


Based on the ESLint docs, if loc isn't provided, it defaults to node.loc so I figured we could just do the same and save ourselves an if statement :)

kentcdodds · 2017-02-01T20:15:02Z

lib/rules/jsx-indent.js

+
+      var parentElementIndent = getNodeIndent(prevToken);
+      if (prevToken.type === 'JSXElement') {
+        parentElementIndent = getOpeningElementIndent(prevToken.openingElement);


Note the recurssion here.

kentcdodds · 2017-02-01T20:16:16Z

tests/lib/rules/jsx-indent.js

-    // as the baseline for the next two, instead of the realizing the entire three
-    // lines are wrong together. See #608
-    /* output: [
+    output: [


Woot! These changes fix this case and we can now auto-fix this!

kentcdodds · 2017-02-01T20:16:57Z

tests/lib/rules/jsx-indent.js

@@ -212,17 +214,6 @@ ruleTester.run('jsx-indent', rule, {
    ].join('\n'),
    parserOptions: parserOptions
  }, {
-    // Literals indentation is not touched


I think that Literal indentation should be touched as it doesn't make a difference in the resulting HTML (different for JSXExpressions, but that doesn't apply here).

kentcdodds · 2017-02-01T20:17:25Z

tests/lib/rules/jsx-indent.js

-    parserOptions: parserOptions
+  // }, {
+  // should we put effort in making this work?
+  // who in their right mind would do this?


Question stands... It would require more logic in the fixer and I don't think it's worth the complexity.

I don't think this is worth supporting.

Great, I went ahead and removed the commented-out code.

kentcdodds · 2017-02-01T20:22:41Z

Pinging those who have touched this rule in one way or another: @yannickcr, @Jorundur, @eelyafi, @jayphelps, @petersendidit, @voxpelli

Thanks friends!

kentcdodds · 2017-02-01T20:52:36Z

Don't merge this yet, just found a bug with the following code:

<div>
  {
    foo &&
      <div>
        <div></div>
      </div>
  }
</div>

We're getting a bad report on the middle-most div. Looking into fixing that.

kentcdodds · 2017-02-01T21:18:18Z

Just noticed that this is actually by design (a few valid tests showing this). I'm going to add a config option to enable this style.

kentcdodds · 2017-02-01T21:31:57Z

Ok, I've updated the PR with documentation for the new configuration option indentLogicalExpressions

kentcdodds · 2017-02-01T21:35:19Z

Oh, and just an argument for the indentLogicalExpressions pattern is the language-babel plugin for Atom (761,282 downloads) has an option to auto-indent JSX and this is how it indents things. Also I personally prefer this indentation style FWIW :)

kentcdodds · 2017-02-01T21:42:01Z

I published my branch as @kentcdodds/eslint-plugin-react if anyone wants to try it out.

kentcdodds · 2017-02-01T21:52:56Z

I've verified that this works on my work project 👍

jayphelps · 2017-02-01T21:58:36Z

Is this multiple fixes to separate indention cases?

kentcdodds · 2017-02-01T22:02:00Z

Is this multiple fixes to separate indention cases?

<ng-sheepishly>Yes, it is...</ng-sheepishly>

Originally, it was to fix a single issue with --fix, but I noticed that in my project we also indent multi-line LogicalExpressions so I slipped that in. I'd definitely prefer to just merge things together (as they are now), but can create a separate PR if people really feel strongly about this...

jayphelps · 2017-02-01T22:05:10Z

It's just a bit hard for me to follow with the various changes for separate things. I don't have enough cycles to fully understand it so I can't give a sign off, but it generally looks good. There were a couple changes you made that are opinionated--you called them out with comments.

kentcdodds · 2017-02-01T22:06:34Z

Understood @jayphelps, if others feel this way, then I'll split things up.

jayphelps · 2017-02-01T22:07:43Z

Could you perhaps edit the original post to include a summary of the changed behaviors?

kentcdodds · 2017-02-01T22:07:19Z

lib/rules/jsx-indent.js

+        parentElementIndent = getOpeningElementIndent(prevToken.openingElement);
+      }
+
+      if (isRightInLogicalExp(node) && indentLogicalExpressions) {


This is the only relevant bit for the indentLogicalExpressions option.

kentcdodds · 2017-02-01T22:09:24Z

lib/rules/jsx-indent.js

+     * @param {ASTNode} node The JSXOpeningElement
+     * @return {Number} the number of indentation characters it should have
+     */
+    function getOpeningElementIndent(node) {


This function came from the JSXOpeningElement function below. I extracted it to make use of it else where. The only changes from what was there before is the two if statements before the var indent = ( assignment.

Thanks for providing this context. This is helpful for reviewers.

kentcdodds · 2017-02-01T22:13:35Z

Updated, let me know if more details would be helpful. Thanks @jayphelps!

kentcdodds · 2017-02-07T23:37:22Z

lib/rules/jsx-indent.js

    return {
      JSXOpeningElement: function(node) {
        var prevToken = sourceCode.getTokenBefore(node);
        if (!prevToken) {
          return;
        }
-        // Use the parent in a list or an array


This code was simply refactored to getOpeningElementIndent

lencioni

This looks great to me. Thanks for the contribution @kentcdodds.

@yannickcr it would be nice to get this in the next release.

lencioni · 2017-02-15T17:50:31Z

lib/rules/jsx-indent.js

@@ -165,8 +184,9 @@ module.exports = {
      } while (token.type === 'JSXText' && /^\s*$/.test(token.value));
      var startLine = node.loc.start.line;
      var endLine = token ? token.loc.end.line : -1;
+      var whitespaceOnly = token ? /\n\s*$/.test(token.value) : false;


This probably doesn't matter, but I think this regex won't match the first line? Should it be /^\s*$/ instead? I'm also not sure what token.value represents, so perhaps the \n is necessary. (see also the regex on line 184)

If these end up being the same, it might not be a bad idea to move them to a constant at the module level.

I just tried out changing it to ^ and it results in an additional extraneous error in one of the test cases. Specifically:

function MyComponent(props) { return ( <div className="foo-bar" id="thing" > Hello world! </div> ) }

Because token.value is actually everything between the > and the </div> (which includes a \n)

lencioni · 2017-02-15T17:52:03Z

lib/rules/jsx-indent.js

+     * @param {ASTNode} node The JSXOpeningElement
+     * @return {Number} the number of indentation characters it should have
+     */
+    function getOpeningElementIndent(node) {


Thanks for providing this context. This is helpful for reviewers.

lencioni · 2017-02-15T17:53:00Z

lib/rules/jsx-indent.js

+      if (prevToken.type === 'JSXText' || prevToken.type === 'Punctuator' && prevToken.value === ',') {
+        prevToken = sourceCode.getNodeByRangeIndex(prevToken.start);
+        prevToken = prevToken.type === 'Literal' ? prevToken.parent : prevToken;
+      // Use the first non-punctuator token in a conditional expression


nit: I find this style of commenting to be pretty awkward. I think this would be best if it was moved inside the conditional branch. Don't worry about this unless you end up touching this again, especially since you didn't write it.

I was in the area and updated the commenting style. I also find it confusing as it is.

lencioni · 2017-02-15T17:54:29Z

tests/lib/rules/jsx-indent.js

-    parserOptions: parserOptions
+  // }, {
+  // should we put effort in making this work?
+  // who in their right mind would do this?


I don't think this is worth supporting.

kentcdodds

I made a few small (comments only) changes based on feedback.

kentcdodds · 2017-02-15T18:23:20Z

lib/rules/jsx-indent.js

@@ -165,8 +184,9 @@ module.exports = {
      } while (token.type === 'JSXText' && /^\s*$/.test(token.value));
      var startLine = node.loc.start.line;
      var endLine = token ? token.loc.end.line : -1;
+      var whitespaceOnly = token ? /\n\s*$/.test(token.value) : false;


I just tried out changing it to ^ and it results in an additional extraneous error in one of the test cases. Specifically:

function MyComponent(props) { return ( <div className="foo-bar" id="thing" > Hello world! </div> ) }

Because token.value is actually everything between the > and the </div> (which includes a \n)

kentcdodds · 2017-02-15T18:24:07Z

lib/rules/jsx-indent.js

+      if (prevToken.type === 'JSXText' || prevToken.type === 'Punctuator' && prevToken.value === ',') {
+        prevToken = sourceCode.getNodeByRangeIndex(prevToken.start);
+        prevToken = prevToken.type === 'Literal' ? prevToken.parent : prevToken;
+      // Use the first non-punctuator token in a conditional expression


I was in the area and updated the commenting style. I also find it confusing as it is.

kentcdodds · 2017-02-15T18:24:57Z

tests/lib/rules/jsx-indent.js

-    parserOptions: parserOptions
+  // }, {
+  // should we put effort in making this work?
+  // who in their right mind would do this?


Great, I went ahead and removed the commented-out code.

kentcdodds force-pushed the pr/jsx-indent-tabs-fix branch 12 times, most recently from 1e7fbe5 to d809e34 Compare February 1, 2017 20:17

kentcdodds commented Feb 1, 2017

View reviewed changes

kentcdodds force-pushed the pr/jsx-indent-tabs-fix branch 2 times, most recently from a226f40 to 83396d3 Compare February 1, 2017 21:31

kentcdodds commented Feb 1, 2017

View reviewed changes

kentcdodds commented Feb 7, 2017

View reviewed changes

lencioni approved these changes Feb 15, 2017

View reviewed changes

lencioni added the enhancement label Feb 15, 2017

lencioni assigned yannickcr Feb 15, 2017

kentcdodds force-pushed the pr/jsx-indent-tabs-fix branch 2 times, most recently from 95a5f8e to fe5c7e3 Compare February 15, 2017 18:24

kentcdodds commented Feb 15, 2017

View reviewed changes

[Fix] jsx-indent with tabs (fixes jsx-eslint#1057)

6e5f688

kentcdodds force-pushed the pr/jsx-indent-tabs-fix branch from fe5c7e3 to 6e5f688 Compare February 15, 2017 18:49

lencioni merged commit a4b6a85 into jsx-eslint:master Feb 16, 2017

kentcdodds deleted the pr/jsx-indent-tabs-fix branch February 16, 2017 00:17

renovate-bot mentioned this pull request Mar 19, 2017

Update dependency eslint-plugin-react to version 6.10.3 singapore/lint-condo#262

Merged

yannickcr mentioned this pull request Mar 20, 2017

v6.10.1 breaks v6.10.0 builds (related to jsx-indent) #1117

Closed

[Fix] jsx-indent with tabs #1058

[Fix] jsx-indent with tabs #1058

Conversation

kentcdodds commented Feb 1, 2017 • edited

kentcdodds 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

kentcdodds commented Feb 1, 2017

kentcdodds commented Feb 1, 2017

kentcdodds commented Feb 1, 2017

kentcdodds commented Feb 1, 2017

kentcdodds commented Feb 1, 2017

kentcdodds commented Feb 1, 2017

kentcdodds commented Feb 1, 2017

jayphelps commented Feb 1, 2017

kentcdodds commented Feb 1, 2017

jayphelps commented Feb 1, 2017 • edited

kentcdodds commented Feb 1, 2017

jayphelps commented Feb 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds commented Feb 1, 2017

Choose a reason for hiding this comment

lencioni 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

kentcdodds 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

kentcdodds commented Feb 1, 2017 •

edited

jayphelps commented Feb 1, 2017 •

edited