From e83600b378a243f5aba4713de4f4aef38017d5b9 Mon Sep 17 00:00:00 2001 From: "Kent C. Dodds" Date: Wed, 1 Feb 2017 10:37:23 -0700 Subject: [PATCH] [Fix] jsx-indent with tabs (fixes #1057) --- lib/rules/jsx-indent.js | 187 +++++++++++++++++++++++----------- tests/lib/rules/jsx-indent.js | 76 +++++++++----- 2 files changed, 177 insertions(+), 86 deletions(-) diff --git a/lib/rules/jsx-indent.js b/lib/rules/jsx-indent.js index 6a75b6f64f..d15d05492e 100644 --- a/lib/rules/jsx-indent.js +++ b/lib/rules/jsx-indent.js @@ -73,18 +73,16 @@ module.exports = { /** * Responsible for fixing the indentation issue fix - * @param {ASTNode} node Node violating the indent rule + * @param {Boolean} rangeToReplace is used to specify the range + * to replace with the correct indentation. * @param {Number} needed Expected indentation character count * @returns {Function} function to be executed by the fixer * @private */ - function getFixerFunction(node, needed) { + function getFixerFunction(rangeToReplace, needed) { return function(fixer) { var indent = Array(needed + 1).join(indentChar); - return fixer.replaceTextRange( - [node.start - node.loc.start.column, node.start], - indent - ); + return fixer.replaceTextRange(rangeToReplace, indent); }; } @@ -93,46 +91,36 @@ module.exports = { * @param {ASTNode} node Node violating the indent rule * @param {Number} needed Expected indentation character count * @param {Number} gotten Indentation character count in the actual node/code - * @param {Object} loc Error line and column location + * @param {Array} rangeToReplace is used in the fixer. + * Defaults to the indent of the start of the node + * @param {Object} loc Error line and column location (defaults to node.loc */ - function report(node, needed, gotten, loc) { + function report(node, needed, gotten, rangeToReplace, loc) { var msgContext = { needed: needed, type: indentType, characters: needed === 1 ? 'character' : 'characters', gotten: gotten }; + rangeToReplace = rangeToReplace || [node.start - node.loc.start.column, node.start]; - if (loc) { - context.report({ - node: node, - loc: loc, - message: MESSAGE, - data: msgContext, - fix: getFixerFunction(node, needed) - }); - } else { - context.report({ - node: node, - message: MESSAGE, - data: msgContext, - fix: getFixerFunction(node, needed) - }); - } + context.report({ + node: node, + loc: loc || node.loc, + message: MESSAGE, + data: msgContext, + fix: getFixerFunction(rangeToReplace, needed) + }); } /** - * Get node indent - * @param {ASTNode} node Node to examine - * @param {Boolean} byLastLine get indent of node's last line - * @param {Boolean} excludeCommas skip comma on start of line - * @return {Number} Indent + * Get the indentation (of the proper indentType) that exists in the source + * @param {String} the source string + * @param {Boolean} whether the line checked should be the last (defaults to the first line) + * @param {Boolean} whether to skip commas in the check (defaults to false) + * @return {Number} the indentation of the indentType that exists on the line */ - function getNodeIndent(node, byLastLine, excludeCommas) { - byLastLine = byLastLine || false; - excludeCommas = excludeCommas || false; - - var src = sourceCode.getText(node, node.loc.start.column + extraColumnStart); + function getIndentFromString(src, byLastLine, excludeCommas) { var lines = src.split('\n'); if (byLastLine) { src = lines[lines.length - 1]; @@ -154,7 +142,24 @@ module.exports = { } /** - * Checks node is the first in its own start line. By default it looks by start line. + * Get node indent + * @param {ASTNode} node Node to examine + * @param {Boolean} byLastLine get indent of node's last line + * @param {Boolean} excludeCommas skip comma on start of line + * @return {Number} Indent + */ + function getNodeIndent(node, byLastLine, excludeCommas) { + byLastLine = byLastLine || false; + excludeCommas = excludeCommas || false; + + var src = sourceCode.getText(node, node.loc.start.column + extraColumnStart); + + return getIndentFromString(src, byLastLine, excludeCommas); + } + + /** + * Checks if the node is the first in its own start line. By default it looks by start line. + * One exception is closing tags with preceeding whitespace * @param {ASTNode} node The node to check * @return {Boolean} true if its the first in the its start line */ @@ -165,8 +170,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; - return startLine !== endLine; + return startLine !== endLine || whitespaceOnly; } /** @@ -218,41 +224,74 @@ module.exports = { } } + /** + * Checks the end of the tag (>) to determine whether it's on its own line + * If so, it verifies the indentation is correct and reports if it is not + * @param {[type]} node [description] + * @param {[type]} startIndent [description] + * @return {[type]} [description] + */ + function checkTagEndIndent(node, startIndent) { + var source = sourceCode.getText(node); + var isTagEndOnOwnLine = /\n\s*\/?>$/.exec(source); + if (isTagEndOnOwnLine) { + var endIndent = getIndentFromString(source, true, false); + if (endIndent !== startIndent) { + var rangeToReplace = [node.end - node.loc.end.column, node.end - 1]; + report(node, startIndent, endIndent, rangeToReplace); + } + } + } + + function getOpeningElementIndent(node) { + var prevToken = sourceCode.getTokenBefore(node); + if (!prevToken) { + return 0; + } + // Use the parent in a list or an array + 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 + } else if (prevToken.type === 'Punctuator' && prevToken.value === ':') { + do { + prevToken = sourceCode.getTokenBefore(prevToken); + } while (prevToken.type === 'Punctuator'); + prevToken = sourceCode.getNodeByRangeIndex(prevToken.start); + while (prevToken.parent && prevToken.parent.type !== 'ConditionalExpression') { + prevToken = prevToken.parent; + } + } + prevToken = prevToken.type === 'JSXExpressionContainer' ? prevToken.expression : prevToken; + + var parentElementIndent = getNodeIndent(prevToken); + if (prevToken.type === 'JSXElement') { + parentElementIndent = getOpeningElementIndent(prevToken.openingElement); + } + + var indent = ( + prevToken.loc.start.line === node.loc.start.line || + isRightInLogicalExp(node) || + isAlternateInConditionalExp(node) + ) ? 0 : indentSize; + return parentElementIndent + indent; + } + return { JSXOpeningElement: function(node) { var prevToken = sourceCode.getTokenBefore(node); if (!prevToken) { return; } - // Use the parent in a list or an array - 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 - } else if (prevToken.type === 'Punctuator' && prevToken.value === ':') { - do { - prevToken = sourceCode.getTokenBefore(prevToken); - } while (prevToken.type === 'Punctuator'); - prevToken = sourceCode.getNodeByRangeIndex(prevToken.start); - while (prevToken.parent && prevToken.parent.type !== 'ConditionalExpression') { - prevToken = prevToken.parent; - } - } - prevToken = prevToken.type === 'JSXExpressionContainer' ? prevToken.expression : prevToken; - - var parentElementIndent = getNodeIndent(prevToken); - var indent = ( - prevToken.loc.start.line === node.loc.start.line || - isRightInLogicalExp(node) || - isAlternateInConditionalExp(node) - ) ? 0 : indentSize; - checkNodesIndent(node, parentElementIndent + indent); + var startIndent = getOpeningElementIndent(node); + checkNodesIndent(node, startIndent); + checkTagEndIndent(node, startIndent); }, JSXClosingElement: function(node) { if (!node.parent) { return; } - var peerElementIndent = getNodeIndent(node.parent.openingElement); + var peerElementIndent = getOpeningElementIndent(node.parent.openingElement); checkNodesIndent(node, peerElementIndent); }, JSXExpressionContainer: function(node) { @@ -261,6 +300,34 @@ module.exports = { } var parentNodeIndent = getNodeIndent(node.parent); checkNodesIndent(node, parentNodeIndent + indentSize); + }, + Literal: function(node) { + if (!node.parent || (node.parent.type !== 'JSXElement' && node.parent.type !== 'JSXExpressionContainer')) { + return; + } + var parentElementIndent = getOpeningElementIndent(node.parent.openingElement); + var expectedIndent = parentElementIndent + indentSize; + var source = sourceCode.getText(node); + var lines = source.split('\n'); + var currentIndex = 0; + lines.forEach(function(line, lineNumber) { + if (line.trim()) { + var lineIndent = getIndentFromString(line); + if (lineIndent !== expectedIndent) { + var lineStart = source.indexOf(line, currentIndex); + var lineIndentStart = line.search(/\S/); + var lineIndentEnd = lineStart + lineIndentStart; + var rangeToReplace = [node.start + lineStart, node.start + lineIndentEnd]; + var locLine = lineNumber + node.loc.start.line; + var loc = { + start: {line: locLine, column: lineIndentStart}, + end: {line: locLine, column: lineIndentEnd} + }; + report(node, expectedIndent, lineIndent, rangeToReplace, loc); + } + } + currentIndex += line.length; + }); } }; diff --git a/tests/lib/rules/jsx-indent.js b/tests/lib/rules/jsx-indent.js index a0072709c1..af9270a4fd 100644 --- a/tests/lib/rules/jsx-indent.js +++ b/tests/lib/rules/jsx-indent.js @@ -51,14 +51,16 @@ ruleTester.run('jsx-indent', rule, { ].join('\n'), options: [0], parserOptions: parserOptions - }, { - code: [ - ' ', - '', - ' ' - ].join('\n'), - options: [-2], - parserOptions: parserOptions + // }, { + // should we put effort in making this work? + // who in their right mind would do this? + // code: [ + // ' ', + // '', + // ' ' + // ].join('\n'), + // options: [-2], + // parserOptions: parserOptions }, { code: [ '', @@ -211,17 +213,6 @@ ruleTester.run('jsx-indent', rule, { '' ].join('\n'), parserOptions: parserOptions - }, { - // Literals indentation is not touched - code: [ - '
', - 'bar
', - ' bar', - ' bar {foo}', - 'bar
', - '
' - ].join('\n'), - parserOptions: parserOptions }, { // Multiline ternary // (colon at the end of the first expression) @@ -459,6 +450,39 @@ ruleTester.run('jsx-indent', rule, { options: ['tab'], parserOptions: parserOptions, errors: [{message: 'Expected indentation of 1 tab character but found 0.'}] + }, { + code: [ + 'function MyComponent(props) {', + '\treturn (', + ' ', + ' Hello world!', + ' ', + '\t)', + '}' + ].join('\n'), + output: [ + 'function MyComponent(props) {', + '\treturn (', + '\t\t', + '\t\t\tHello world!', + '\t\t', + '\t)', + '}' + ].join('\n'), + options: ['tab'], + parserOptions: parserOptions, + errors: [ + {message: 'Expected indentation of 2 tab characters but found 0.'}, + {message: 'Expected indentation of 2 tab characters but found 0.'}, + {message: 'Expected indentation of 3 tab characters but found 0.'}, + {message: 'Expected indentation of 2 tab characters but found 0.'} + ] }, { code: [ 'function App() {', @@ -505,11 +529,7 @@ ruleTester.run('jsx-indent', rule, { ' );', '}' ].join('\n'), - // The detection logic only thinks is indented wrong, not the other - // two lines following. I *think* because it incorrectly uses 's indention - // as the baseline for the next two, instead of the realizing the entire three - // lines are wrong together. See #608 - /* output: [ + output: [ 'function App() {', ' return (', ' ', @@ -517,10 +537,14 @@ ruleTester.run('jsx-indent', rule, { ' ', ' );', '}' - ].join('\n'), */ + ].join('\n'), options: [2], parserOptions: parserOptions, - errors: [{message: 'Expected indentation of 4 space characters but found 0.'}] + errors: [ + {message: 'Expected indentation of 4 space characters but found 0.'}, + {message: 'Expected indentation of 6 space characters but found 2.'}, + {message: 'Expected indentation of 4 space characters but found 0.'} + ] }, { code: [ '',