Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Nov 18, 2017
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8f3dc55
Add failing test
jseminck Nov 16, 2017
6d50fb6
Fix test by using the same isNodeFirstInLine function found in jsx-cl…
jseminck Nov 16, 2017
c51087c
Extract isNodeFirstInLine to astUtil function
jseminck Nov 16, 2017
acc4f24
Use the new function also in jsx-indent-props
jseminck Nov 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 4 additions & 27 deletions lib/rules/jsx-closing-tag-location.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
*/
'use strict';

const astUtil = require('../util/ast');

// ------------------------------------------------------------------------------
// Rule Definition
// ------------------------------------------------------------------------------
Expand All @@ -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) {
Expand All @@ -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.';
Expand All @@ -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
Expand Down
18 changes: 3 additions & 15 deletions lib/rules/jsx-indent-props.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
*/
'use strict';

const astUtil = require('../util/ast');

// ------------------------------------------------------------------------------
// Rule Definition
// ------------------------------------------------------------------------------
Expand Down Expand Up @@ -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
Expand All @@ -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);
}
Expand Down
20 changes: 3 additions & 17 deletions lib/rules/jsx-indent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
*/
'use strict';

const astUtil = require('../util/ast');

// ------------------------------------------------------------------------------
// Rule Definition
// ------------------------------------------------------------------------------
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -209,7 +195,7 @@ module.exports = {
const isCorrectAlternateInCondExp = isAlternateInConditionalExp(node) && (nodeIndent - indent) === 0;
if (
nodeIndent !== indent &&
isNodeFirstInLine(node) &&
astUtil.isNodeFirstInLine(context, node) &&
!isCorrectRightInLogicalExp &&
!isCorrectAlternateInCondExp
) {
Expand Down
69 changes: 48 additions & 21 deletions lib/util/ast.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I re-ordered the functions alphabetically.

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.
Expand Down Expand Up @@ -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
};
11 changes: 11 additions & 0 deletions tests/lib/rules/jsx-indent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.'}
]
}]
});