-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Add new no-children-prop rule (Fixes #720) #722
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
# Prevent passing of children as props (no-children-prop) | ||
|
||
Children should always be actual children, not passed in as a prop. | ||
|
||
When using JSX, the children should be nested between the opening and closing | ||
tags. When not using JSX, the children should be passed as additional | ||
arguments to `React.createElement`. | ||
|
||
## Rule Details | ||
|
||
The following patterns are considered warnings: | ||
|
||
```jsx | ||
<div children='Children' /> | ||
|
||
<MyComponent children={<AnotherComponent />} /> | ||
<MyComponent children={['Child 1', 'Child 2']} /> | ||
|
||
React.createElement("div", { children: 'Children' }) | ||
``` | ||
|
||
The following patterns are not considered warnings: | ||
|
||
```jsx | ||
<div>Children</div> | ||
|
||
<MyComponent>Children</MyComponent> | ||
|
||
<MyComponent> | ||
<span>Child 1</span> | ||
<span>Child 2</span> | ||
</MyComponent> | ||
|
||
React.createElement("div", {}, 'Children') | ||
React.createElement("div", 'Child 1', 'Child 2') | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
/** | ||
* @fileoverview Prevent passing of children as props | ||
* @author Benjamin Stepp | ||
*/ | ||
'use strict'; | ||
|
||
// ------------------------------------------------------------------------------ | ||
// Helpers | ||
// ------------------------------------------------------------------------------ | ||
|
||
/** | ||
* Checks if the node is a createElement call with a props literal. | ||
* @param {ASTNode} node - The AST node being checked. | ||
* @returns {Boolean} - True if node is a createElement call with a props | ||
* object literal, False if not. | ||
*/ | ||
function isCreateElementWithProps(node) { | ||
return node.callee | ||
&& node.callee.type === 'MemberExpression' | ||
&& node.callee.property.name === 'createElement' | ||
&& node.arguments.length > 1 | ||
&& node.arguments[1].type === 'ObjectExpression'; | ||
} | ||
|
||
// ------------------------------------------------------------------------------ | ||
// Rule Definition | ||
// ------------------------------------------------------------------------------ | ||
|
||
module.exports = { | ||
meta: { | ||
docs: { | ||
description: 'Prevent passing of children as props.', | ||
category: 'Best Practices', | ||
recommended: false | ||
}, | ||
schema: [] | ||
}, | ||
create: function(context) { | ||
return { | ||
JSXAttribute: function(node) { | ||
if (node.name.name !== 'children') { | ||
return; | ||
} | ||
|
||
context.report({ | ||
node: node, | ||
message: 'Do not pass children as props. Instead, nest children between the opening and closing tags.' | ||
}); | ||
}, | ||
CallExpression: function(node) { | ||
if (!isCreateElementWithProps(node)) { | ||
return; | ||
} | ||
|
||
var props = node.arguments[1].properties; | ||
var childrenProp = props.find(function(prop) { | ||
return prop.key.name === 'children'; | ||
}); | ||
|
||
if (childrenProp) { | ||
context.report({ | ||
node: node, | ||
message: 'Do not pass children as props. Instead, pass them as additional arguments to React.createElement.' | ||
}); | ||
} | ||
} | ||
}; | ||
} | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,237 @@ | ||
/** | ||
* @fileoverview Tests for no-children-prop | ||
* @author Benjamin Stepp | ||
*/ | ||
|
||
'use strict'; | ||
|
||
// ----------------------------------------------------------------------------- | ||
// Requirements | ||
// ----------------------------------------------------------------------------- | ||
|
||
var rule = require('../../../lib/rules/no-children-prop'); | ||
var RuleTester = require('eslint').RuleTester; | ||
|
||
var parserOptions = { | ||
ecmaVersion: 6, | ||
ecmaFeatures: { | ||
jsx: true | ||
} | ||
}; | ||
|
||
var JSX_ERROR = 'Do not pass children as props. Instead, nest children between \ | ||
the opening and closing tags.'; | ||
var CREATE_ELEMENT_ERROR = 'Do not pass children as props. Instead, pass them \ | ||
as additional arguments to React.createElement.'; | ||
|
||
// ----------------------------------------------------------------------------- | ||
// Tests | ||
// ----------------------------------------------------------------------------- | ||
|
||
var ruleTester = new RuleTester(); | ||
ruleTester.run('no-children-prop', rule, { | ||
valid: [ | ||
{ | ||
code: '<div />;', | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: '<div></div>;', | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: 'React.createElement("div", {});', | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: 'React.createElement("div", undefined);', | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: '<div className="class-name"></div>;', | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: 'React.createElement("div", {className: "class-name"});', | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: '<div>Children</div>;', | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: 'React.createElement("div", "Children");', | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: 'React.createElement("div", {}, "Children");', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be nice to include an example with multiple children. e.g. React.createElement("div", {}, "Child 1", "Child 2"); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, a valid case where there is at least one non-children prop with React.createElement and JSX. |
||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: 'React.createElement("div", undefined, "Children");', | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: '<div className="class-name">Children</div>;', | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: 'React.createElement("div", {className: "class-name"}, "Children");', | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: '<div><div /></div>;', | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: 'React.createElement("div", React.createElement("div"));', | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: 'React.createElement("div", {}, React.createElement("div"));', | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: 'React.createElement("div", undefined, React.createElement("div"));', | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: '<div><div /><div /></div>;', | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: 'React.createElement("div", React.createElement("div"), React.createElement("div"));', | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: 'React.createElement("div", {}, React.createElement("div"), React.createElement("div"));', | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: 'React.createElement("div", undefined, React.createElement("div"), React.createElement("div"));', | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: 'React.createElement("div", [React.createElement("div"), React.createElement("div")]);', | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: 'React.createElement("div", {}, [React.createElement("div"), React.createElement("div")]);', | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: 'React.createElement("div", undefined, [React.createElement("div"), React.createElement("div")]);', | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: '<MyComponent />', | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: 'React.createElement(MyComponent);', | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: 'React.createElement(MyComponent, {});', | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: 'React.createElement(MyComponent, undefined);', | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: '<MyComponent>Children</MyComponent>;', | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: 'React.createElement(MyComponent, "Children");', | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: 'React.createElement(MyComponent, {}, "Children");', | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: 'React.createElement(MyComponent, undefined, "Children");', | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: '<MyComponent className="class-name"></MyComponent>;', | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: 'React.createElement(MyComponent, {className: "class-name"});', | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: '<MyComponent className="class-name">Children</MyComponent>;', | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: 'React.createElement(MyComponent, {className: "class-name"}, "Children");', | ||
parserOptions: parserOptions | ||
} | ||
], | ||
invalid: [ | ||
{ | ||
code: '<div children="Children" />;', | ||
errors: [{message: JSX_ERROR}], | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: '<div children={<div />} />;', | ||
errors: [{message: JSX_ERROR}], | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: '<div children={[<div />, <div />]} />;', | ||
errors: [{message: JSX_ERROR}], | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: '<div children="Children">Children</div>;', | ||
errors: [{message: JSX_ERROR}], | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: 'React.createElement("div", {children: "Children"});', | ||
errors: [{message: CREATE_ELEMENT_ERROR}], | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: 'React.createElement("div", {children: "Children"}, "Children");', | ||
errors: [{message: CREATE_ELEMENT_ERROR}], | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: 'React.createElement("div", {children: React.createElement("div")});', | ||
errors: [{message: CREATE_ELEMENT_ERROR}], | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: 'React.createElement("div", {children: [React.createElement("div"), React.createElement("div")]});', | ||
errors: [{message: CREATE_ELEMENT_ERROR}], | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: '<MyComponent children="Children" />', | ||
errors: [{message: JSX_ERROR}], | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: 'React.createElement(MyComponent, {children: "Children"});', | ||
errors: [{message: CREATE_ELEMENT_ERROR}], | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: '<MyComponent className="class-name" children="Children" />;', | ||
errors: [{message: JSX_ERROR}], | ||
parserOptions: parserOptions | ||
}, | ||
{ | ||
code: 'React.createElement(MyComponent, {children: "Children", className: "class-name"});', | ||
errors: [{message: CREATE_ELEMENT_ERROR}], | ||
parserOptions: parserOptions | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This rule should also warn when Components receive a <MyComponent children='Children' /> |
||
] | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should go in the recommended config as well. Thoughts @yannickcr?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but that change could be considered breaking whereas this change is minor. If this is going to make it into v6 then that's fine, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is ready for v6 so might as well include it in recommended.