Merge pull request #1539 from jseminck/jsx-indent-bug

Fix alignment bug in jsx-indent (and extract duplicate code)

Author: ljharb

lib/rules/jsx-closing-tag-location.js

@@ -4,6 +4,8 @@
  */
 'use strict';
 
+const astUtil = require('../util/ast');
+
 // ------------------------------------------------------------------------------
 // Rule Definition
 // ------------------------------------------------------------------------------
@@ -18,31 +20,6 @@ module.exports = {
   },
 
   create: function(context) {
-    const sourceCode = context.getSourceCode();
-
-    /**
-     * Checks if the node is the first in its line, excluding whitespace.
-     * @param {ASTNode} node The node to check
-     * @return {Boolean} true if its the first node in its line
-     */
-    function isNodeFirstInLine(node) {
-      let token = node;
-      let lines;
-      do {
-        token = sourceCode.getTokenBefore(token);
-        lines = token.type === 'JSXText'
-          ? token.value.split('\n')
-          : null;
-      } while (
-        token.type === 'JSXText' &&
-        /^\s*$/.test(lines[lines.length - 1])
-      );
-
-      const startLine = node.loc.start.line;
-      const endLine = token ? token.loc.end.line : -1;
-      return startLine !== endLine;
-    }
-
     return {
       JSXClosingElement: function(node) {
         if (!node.parent) {
@@ -59,7 +36,7 @@ module.exports = {
         }
 
         let message;
-        if (!isNodeFirstInLine(node)) {
+        if (!astUtil.isNodeFirstInLine(context, node)) {
           message = 'Closing tag of a multiline JSX expression must be on its own line.';
         } else {
           message = 'Expected closing tag to match indentation of opening.';
@@ -71,7 +48,7 @@ module.exports = {
           message,
           fix: function(fixer) {
             const indent = Array(opening.loc.start.column + 1).join(' ');
-            if (isNodeFirstInLine(node)) {
+            if (astUtil.isNodeFirstInLine(context, node)) {
               return fixer.replaceTextRange(
                 [node.range[0] - node.loc.start.column, node.range[0]],
                 indent

lib/rules/jsx-indent-props.js

@@ -29,6 +29,8 @@
  */
 'use strict';
 
+const astUtil = require('../util/ast');
+
 // ------------------------------------------------------------------------------
 // Rule Definition
 // ------------------------------------------------------------------------------
@@ -136,20 +138,6 @@ module.exports = {
       return indent ? indent[0].length : 0;
     }
 
-    /**
-     * Checks node is the first in its own start line. By default it looks by start line.
-     * @param {ASTNode} node The node to check
-     * @param {Boolean} [byEndLocation] Lookup based on start position or end
-     * @return {Boolean} true if its the first in the its start line
-     */
-    function isNodeFirstInLine(node, byEndLocation) {
-      const firstToken = byEndLocation === true ? sourceCode.getLastToken(node, 1) : sourceCode.getTokenBefore(node);
-      const startLine = byEndLocation === true ? node.loc.end.line : node.loc.start.line;
-      const endLine = firstToken ? firstToken.loc.end.line : -1;
-
-      return startLine !== endLine;
-    }
-
     /**
      * Check indent for nodes list
      * @param {ASTNode[]} nodes list of node objects
@@ -161,7 +149,7 @@ module.exports = {
         const nodeIndent = getNodeIndent(node, false, excludeCommas);
         if (
           node.type !== 'ArrayExpression' && node.type !== 'ObjectExpression' &&
-          nodeIndent !== indent && isNodeFirstInLine(node)
+          nodeIndent !== indent && astUtil.isNodeFirstInLine(context, node)
         ) {
           report(node, indent, nodeIndent);
         }

lib/rules/jsx-indent.js

@@ -29,6 +29,8 @@
  */
 'use strict';
 
+const astUtil = require('../util/ast');
+
 // ------------------------------------------------------------------------------
 // Rule Definition
 // ------------------------------------------------------------------------------
@@ -152,22 +154,6 @@ module.exports = {
       return indent ? indent[0].length : 0;
     }
 
-    /**
-     * Checks node is the first in its own start line. By default it looks by start line.
-     * @param {ASTNode} node The node to check
-     * @return {Boolean} true if its the first in the its start line
-     */
-    function isNodeFirstInLine(node) {
-      let token = node;
-      do {
-        token = sourceCode.getTokenBefore(token);
-      } while (token.type === 'JSXText' && /^\s*$/.test(token.value));
-      const startLine = node.loc.start.line;
-      const endLine = token ? token.loc.end.line : -1;
-
-      return startLine !== endLine;
-    }
-
     /**
      * Check if the node is the right member of a logical expression
      * @param {ASTNode} node The node to check
@@ -209,7 +195,7 @@ module.exports = {
       const isCorrectAlternateInCondExp = isAlternateInConditionalExp(node) && (nodeIndent - indent) === 0;
       if (
         nodeIndent !== indent &&
-        isNodeFirstInLine(node) &&
+        astUtil.isNodeFirstInLine(context, node) &&
         !isCorrectRightInLogicalExp &&
         !isCorrectAlternateInCondExp
       ) {

lib/util/ast.js

@@ -3,6 +3,30 @@
  */
 'use strict';
 
+/**
+ * Find a return statment in the current node
+ *
+ * @param {ASTNode} ASTnode The AST node being checked
+ */
+function findReturnStatement(node) {
+  if (
+    (!node.value || !node.value.body || !node.value.body.body) &&
+    (!node.body || !node.body.body)
+  ) {
+    return false;
+  }
+
+  const bodyNodes = (node.value ? node.value.body.body : node.body.body);
+
+  let i = bodyNodes.length - 1;
+  for (; i >= 0; i--) {
+    if (bodyNodes[i].type === 'ReturnStatement') {
+      return bodyNodes[i];
+    }
+  }
+  return false;
+}
+
 /**
  * Get properties name
  * @param {Object} node - Property.
@@ -36,31 +60,34 @@ function getComponentProperties(node) {
 }
 
 /**
- * Find a return statment in the current node
- *
- * @param {ASTNode} ASTnode The AST node being checked
- */
-function findReturnStatement(node) {
-  if (
-    (!node.value || !node.value.body || !node.value.body.body) &&
-    (!node.body || !node.body.body)
-  ) {
-    return false;
-  }
-
-  const bodyNodes = (node.value ? node.value.body.body : node.body.body);
+     * Checks if the node is the first in its line, excluding whitespace.
+     * @param {Object} context The node to check
+     * @param {ASTNode} node The node to check
+     * @return {Boolean} true if its the first node in its line
+     */
+function isNodeFirstInLine(context, node) {
+  const sourceCode = context.getSourceCode();
+  let token = node;
+  let lines;
+  do {
+    token = sourceCode.getTokenBefore(token);
+    lines = token.type === 'JSXText'
+      ? token.value.split('\n')
+      : null;
+  } while (
+    token.type === 'JSXText' &&
+        /^\s*$/.test(lines[lines.length - 1])
+  );
 
-  let i = bodyNodes.length - 1;
-  for (; i >= 0; i--) {
-    if (bodyNodes[i].type === 'ReturnStatement') {
-      return bodyNodes[i];
-    }
-  }
-  return false;
+  const startLine = node.loc.start.line;
+  const endLine = token ? token.loc.end.line : -1;
+  return startLine !== endLine;
 }
 
+
 module.exports = {
+  findReturnStatement: findReturnStatement,
   getPropertyName: getPropertyName,
   getComponentProperties: getComponentProperties,
-  findReturnStatement: findReturnStatement
+  isNodeFirstInLine: isNodeFirstInLine
 };

tests/lib/rules/jsx-indent.js

@@ -963,5 +963,16 @@ ruleTester.run('jsx-indent', rule, {
     errors: [
       {message: 'Expected indentation of 4 space characters but found 0.'}
     ]
+  }, {
+    code: [
+      '<p>',
+      '    <div>',
+      '        <SelfClosingTag />Text',
+      '  </div>',
+      '</p>'
+    ].join('\n'),
+    errors: [
+      {message: 'Expected indentation of 4 space characters but found 2.'}
+    ]
   }]
 });