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 max props per line updates #882

Merged
merged 4 commits into from
Feb 15, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
34 changes: 30 additions & 4 deletions docs/rules/jsx-max-props-per-line.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,16 @@ Limiting the maximum of props on a single line can improve readability.

## Rule Details

This rule checks all JSX elements and verifies that the number of props per line do not exceed the maximum allowed. A spread attribute counts as one prop. This rule is off by default and when on the default maximum of props on one line is `1`.
This rule checks all JSX elements and verifies that the number of props per line do not exceed the maximum allowed. Props are considered to be in a new line if there is a line break between the start of the prop and the end of the previous prop. A spread attribute counts as one prop. This rule is off by default and when on the default maximum of props on one line is `1`.

The following patterns are considered warnings:

```jsx
<Hello lastName="Smith" firstName="John" />;

<Hello foo={{
bar
}} baz />;
```

The following patterns are not considered warnings:
Expand All @@ -31,7 +35,7 @@ The following patterns are not considered warnings:

```js
...
"jsx-max-props-per-line": [<enabled>, { "maximum": <number> }]
"jsx-max-props-per-line": [<enabled>, { "maximum": <number>, "when": <string> }]
...
```

Expand All @@ -42,20 +46,42 @@ Maximum number of props allowed on a single line. Default to `1`.
The following patterns are considered warnings:

```jsx
// [1, {maximum: 2}]
// [1, { "maximum": 2 }]
<Hello firstName="John" lastName="Smith" tel={5555555} />;
```

The following patterns are not considered warnings:

```jsx
// [1, {maximum: 2}]
// [1, { "maximum": 2 }]
<Hello
firstName="John" lastName="Smith"
tel={5555555}
/>;
```

### `when`

Possible values:
- `always` (default) - Always check for max props per line.
- `multiline` - Only check for max props per line when jsx tag spans multiple lines.

The following patterns are considered warnings:
```jsx
// [1, { "when": "always" }]
<Hello firstName="John" lastName="Smith" />
```

The following patterns are not considered warnings:
```jsx
// [1, { "when": "multiline" }]
<Hello firstName="John" lastName="Smith" />
<Hello
firstName="John"
lastName="Smith"
/>
```

## When not to use

If you are not using JSX then you can disable this rule.
42 changes: 25 additions & 17 deletions lib/rules/jsx-max-props-per-line.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@

'use strict';

var has = require('has');

// ------------------------------------------------------------------------------
// Rule Definition
// ------------------------------------------------------------------------------
Expand All @@ -25,6 +23,10 @@ module.exports = {
maximum: {
type: 'integer',
minimum: 1
},
when: {
type: 'string',
enum: ['always', 'multiline']
}
}
}]
Expand All @@ -35,6 +37,7 @@ module.exports = {
var sourceCode = context.getSourceCode();
var configuration = context.options[0] || {};
var maximum = configuration.maximum || 1;
var when = configuration.when || 'always';

function getPropName(propNode) {
if (propNode.type === 'JSXSpreadAttribute') {
Expand All @@ -45,30 +48,35 @@ module.exports = {

return {
JSXOpeningElement: function (node) {
var props = {};
if (!node.attributes.length) {
return;
}

if (when === 'multiline' && node.loc.start.line === node.loc.end.line) {
return;
}

node.attributes.forEach(function(decl) {
var line = decl.loc.start.line;
if (props[line]) {
props[line].push(decl);
var firstProp = node.attributes[0];
var linePartitionedProps = [[firstProp]];

node.attributes.reduce(function(last, decl) {
if (last.loc.end.line === decl.loc.start.line) {
linePartitionedProps[linePartitionedProps.length - 1].push(decl);
} else {
props[line] = [decl];
linePartitionedProps.push([decl]);
}
return decl;
});

for (var line in props) {
if (!has(props, line)) {
continue;
}
if (props[line].length > maximum) {
var name = getPropName(props[line][maximum]);
linePartitionedProps.forEach(function(propsInLine) {
if (propsInLine.length > maximum) {
var name = getPropName(propsInLine[maximum]);
context.report({
node: props[line][maximum],
node: propsInLine[maximum],
message: 'Prop `' + name + '` must be placed on a new line'
});
break;
}
}
});
}
};
}
Expand Down
74 changes: 74 additions & 0 deletions tests/lib/rules/jsx-max-props-per-line.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,27 @@ var parserOptions = {
var ruleTester = new RuleTester();
ruleTester.run('jsx-max-props-per-line', rule, {
valid: [{
code: '<App />',
parserOptions: parserOptions
}, {
code: '<App foo />',
parserOptions: parserOptions
}, {
code: '<App foo bar />',
options: [{maximum: 2}],
parserOptions: parserOptions
}, {
code: '<App foo bar />',
options: [{when: 'multiline'}],
parserOptions: parserOptions
}, {
code: '<App foo {...this.props} />',
options: [{when: 'multiline'}],
parserOptions: parserOptions
}, {
code: '<App foo bar baz />',
options: [{maximum: 2, when: 'multiline'}],
parserOptions: parserOptions
}, {
code: '<App {...this.props} bar />',
options: [{maximum: 2}],
Expand Down Expand Up @@ -89,5 +104,64 @@ ruleTester.run('jsx-max-props-per-line', rule, {
].join('\n'),
errors: [{message: 'Prop `this.props` must be placed on a new line'}],
parserOptions: parserOptions
}, {
code: [
'<App',
' foo={{',
Copy link
Member

Choose a reason for hiding this comment

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

Let's also include some tests where the tagname and first prop are on the same line, but there's another prop. Also, please add variants of all tests that include spread props

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure thing

' }} bar',
'/>'
].join('\n'),
errors: [{message: 'Prop `bar` must be placed on a new line'}],
parserOptions: parserOptions
}, {
code: [
'<App foo={{',
'}} bar />'
].join('\n'),
errors: [{message: 'Prop `bar` must be placed on a new line'}],
parserOptions: parserOptions
}, {
code: [
'<App foo bar={{',
'}} baz />'
].join('\n'),
options: [{maximum: 2}],
errors: [{message: 'Prop `baz` must be placed on a new line'}],
parserOptions: parserOptions
}, {
code: [
'<App foo={{',
'}} {...rest} />'
].join('\n'),
errors: [{message: 'Prop `rest` must be placed on a new line'}],
parserOptions: parserOptions
}, {
code: [
'<App {',
' ...this.props',
'} bar />'
].join('\n'),
errors: [{message: 'Prop `bar` must be placed on a new line'}],
parserOptions: parserOptions
}, {
code: [
'<App {',
' ...this.props',
'} {',
' ...rest',
'} />'
].join('\n'),
errors: [{message: 'Prop `rest` must be placed on a new line'}],
parserOptions: parserOptions
}, {
code: [
'<App',
' foo={{',
' }} bar baz',
'/>'
].join('\n'),
options: [{maximum: 2}],
errors: [{message: 'Prop `baz` must be placed on a new line'}],
parserOptions: parserOptions
}]
});