Skip to content
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

[Fix] jsx-indent with tabs #1058

Merged
merged 1 commit into from
Feb 16, 2017
Merged

[Fix] jsx-indent with tabs #1058

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
62 changes: 62 additions & 0 deletions docs/rules/jsx-indent.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,68 @@ The following patterns are not warnings:
</App>
```

#### indentLogicalExpressions

```js
...
"react/jsx-indent": [<enabled>, 'tab'|<number>, {indentLogicalExpressions: true}]
...
```

By default this is set to false. When enabled, an additional indentation is required when the JSX is the right of a LogicalExpression

The following patterns are considered warnings:

```jsx
// 2 spaces indentation with indentLogicalExpressions as false
// [2, 2, {indentLogicalExpressions: false}]
<App>
{
condition &&
<Container>
<Child></Child>
</Container>
}
</App>

// 2 spaces indentation with indentLogicalExpressions as true
// [2, 2, {indentLogicalExpressions: true}]
<App>
{
condition &&
<Container>
<Child></Child>
</Container>
}
</App>
```

The following patterns are not warnings:

```jsx
// 2 spaces indentation with indentLogicalExpressions as true
// [2, 2, {indentLogicalExpressions: true}]
<App>
{
condition &&
<Container>
<Child></Child>
</Container>
}
</App>

// 2 spaces indentation with indentLogicalExpressions as false
// [2, 2, {indentLogicalExpressions: false}]
<App>
{
condition &&
<Container>
<Child></Child>
</Container>
}
</App>
```

## When not to use

If you are not using JSX then you can disable this rule.
209 changes: 149 additions & 60 deletions lib/rules/jsx-indent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,14 @@ module.exports = {
}, {
type: 'integer'
}]
}, {
type: 'object',
properties: {
indentLogicalExpressions: {
type: 'boolean'
}
},
additionalProperties: false
}]
},

Expand All @@ -56,6 +64,7 @@ module.exports = {
var extraColumnStart = 0;
var indentType = 'space';
var indentSize = 4;
var indentLogicalExpressions = false;

var sourceCode = context.getSourceCode();

Expand All @@ -67,24 +76,25 @@ module.exports = {
indentSize = context.options[0];
indentType = 'space';
}
if (context.options[1]) {
indentLogicalExpressions = context.options[1].indentLogicalExpressions || false;
}
}

var indentChar = indentType === 'space' ? ' ' : '\t';

/**
* 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);
};
}

Expand All @@ -93,46 +103,38 @@ 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.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 :)

* 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} src the source string
* @param {Boolean} byLastLine whether the line checked should be the last
* Defaults to the first line
* @param {Boolean} excludeCommas 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];
Expand All @@ -154,7 +156,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
*/
Expand All @@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

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 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)


return startLine !== endLine;
return startLine !== endLine || whitespaceOnly;
}

/**
Expand Down Expand Up @@ -218,41 +238,82 @@ 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 {ASTNode} node The node to check
* @param {Number} startIndent The indentation of the start of the tag
*/
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);
}
}
}

/**
* Gets what the JSXOpeningElement's indentation should be
* @param {ASTNode} node The JSXOpeningElement
* @return {Number} the number of indentation characters it should have
*/
function getOpeningElementIndent(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.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

var prevToken = sourceCode.getTokenBefore(node);
if (!prevToken) {
return 0;
}
if (prevToken.type === 'JSXText' || prevToken.type === 'Punctuator' && prevToken.value === ',') {
// Use the parent in a list or an array
prevToken = sourceCode.getNodeByRangeIndex(prevToken.start);
prevToken = prevToken.type === 'Literal' ? prevToken.parent : prevToken;
} else if (prevToken.type === 'Punctuator' && prevToken.value === ':') {
// Use the first non-punctuator token in a conditional expression
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);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note the recurssion here.

}

if (isRightInLogicalExp(node) && indentLogicalExpressions) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only relevant bit for the indentLogicalExpressions option.

parentElementIndent += indentSize;
}

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) {
Expand All @@ -261,6 +322,34 @@ module.exports = {
}
var parentNodeIndent = getNodeIndent(node.parent);
checkNodesIndent(node, parentNodeIndent + indentSize);
},
Literal: function(node) {
if (!node.parent || node.parent.type !== 'JSXElement') {
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;
});
}
};

Expand Down
Loading