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.

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

jsx-first-prop-new-line: autofix #886

Merged
merged 11 commits into from
Nov 4, 2016
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ Finally, enable all of the rules that you would like to use. Use [our preset](#
* [react/jsx-curly-spacing](docs/rules/jsx-curly-spacing.md): Enforce or disallow spaces inside of curly braces in JSX attributes (fixable)
* [react/jsx-equals-spacing](docs/rules/jsx-equals-spacing.md): Enforce or disallow spaces around equal signs in JSX attributes (fixable)
* [react/jsx-filename-extension](docs/rules/jsx-filename-extension.md): Restrict file extensions that may contain JSX
* [react/jsx-first-prop-new-line](docs/rules/jsx-first-prop-new-line.md): Enforce position of the first prop in JSX
* [react/jsx-first-prop-new-line](docs/rules/jsx-first-prop-new-line.md): Enforce position of the first prop in JSX (fixable)
* [react/jsx-handler-names](docs/rules/jsx-handler-names.md): Enforce event handler naming conventions in JSX
* [react/jsx-indent](docs/rules/jsx-indent.md): Validate JSX indentation
* [react/jsx-indent-props](docs/rules/jsx-indent-props.md): Validate props indentation in JSX (fixable)
Expand Down
2 changes: 2 additions & 0 deletions docs/rules/jsx-first-prop-new-line.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

Ensure correct position of the first property.

**Fixable:** This rule is automatically fixable using the `--fix` flag on the command line. However, fix does not include indentation. Please rerun lint to correct those errors.

## Rule Details

This rule checks whether the first property of all JSX elements is correctly placed. There are three possible configurations:
Expand Down
14 changes: 11 additions & 3 deletions lib/rules/jsx-first-prop-new-line.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ module.exports = {
category: 'Stylistic Issues',
recommended: false
},
fixable: 'code',

schema: [{
enum: ['always', 'never', 'multiline', 'multiline-multiprop']
Expand All @@ -35,20 +36,27 @@ module.exports = {
(configuration === 'multiline-multiprop' && isMultilineJSX(node) && node.attributes.length > 1) ||
(configuration === 'always')
) {
node.attributes.forEach(function(decl) {
node.attributes.some(function(decl) {
if (decl.loc.start.line === node.loc.start.line) {
context.report({
node: decl,
message: 'Property should be placed on a new line'
message: 'Property should be placed on a new line',
fix: function(fixer) {
return fixer.replaceTextRange([node.name.end, decl.start], '\n');
}
});
}
return true;
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't we only return true inside the if? otherwise it will only ever run on the first value of node.attributes - is that intended?

Copy link
Contributor Author

@snowypowers snowypowers Nov 3, 2016

Choose a reason for hiding this comment

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

Yes, it is intentional. This rule only concerns the first prop for each React element so we should not be checking anything else after the first element.

Copy link
Member

Choose a reason for hiding this comment

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

I guess the some then is just a nicer way of doing [0] and then having to check if it's defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, its somewhat cleaner than adding && node.attributes.length > 0. Matter of preference.

Copy link
Member

Choose a reason for hiding this comment

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

sounds good 👍

});
} else if (configuration === 'never' && node.attributes.length > 0) {
var firstNode = node.attributes[0];
if (node.loc.start.line < firstNode.loc.start.line) {
context.report({
node: firstNode,
message: 'Property should be placed on the same line as the component declaration'
message: 'Property should be placed on the same line as the component declaration',
fix: function(fixer) {
return fixer.replaceTextRange([node.name.end, firstNode.start], ' ');
}
});
return;
}
Expand Down
31 changes: 28 additions & 3 deletions tests/lib/rules/jsx-first-prop-new-line.js
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,11 @@ ruleTester.run('jsx-first-prop-new-line', rule, {

invalid: [
{
code: '<Foo prop="one" />',
code: '<Foo propOne="one" propTwo="two" />',
output: [
'<Foo',
'propOne="one" propTwo="two" />'
].join('\n'),
options: ['always'],
errors: [{message: 'Property should be placed on a new line'}],
parser: parserOptions
Expand All @@ -163,15 +167,26 @@ ruleTester.run('jsx-first-prop-new-line', rule, {
' propTwo="two"',
'/>'
].join('\n'),
output: [
'<Foo',
'propOne="one"',
' propTwo="two"',
'/>'
].join('\n'),
options: ['always'],
errors: [{message: 'Property should be placed on a new line'}],
parser: parserOptions
},
{
code: [
'<Foo',
' propOne="one"',
' propTwo="two"',
' propOne="one"',
' propTwo="two"',
'/>'
].join('\n'),
output: [
'<Foo propOne="one"',
' propTwo="two"',
'/>'
].join('\n'),
options: ['never'],
Expand All @@ -183,6 +198,11 @@ ruleTester.run('jsx-first-prop-new-line', rule, {
'<Foo prop={{',
'}} />'
].join('\n'),
output: [
'<Foo',
'prop={{',
'}} />'
].join('\n'),
options: ['multiline'],
errors: [{message: 'Property should be placed on a new line'}],
parser: parserOptions
Expand All @@ -192,6 +212,11 @@ ruleTester.run('jsx-first-prop-new-line', rule, {
'<Foo bar={{',
'}} baz />'
].join('\n'),
output: [
'<Foo',
'bar={{',
'}} baz />'
].join('\n'),
options: ['multiline-multiprop'],
errors: [{message: 'Property should be placed on a new line'}],
parser: parserOptions
Expand Down