Add new no-children-prop rule (Fixes #720) #722
Conversation
benstepp
force-pushed
the
bs-no-children-prop
branch
from
dc90659
to
3f1d0b2
Compare
|
LGTM |
|
|
||
| The following patterns are considered warnings: | ||
|
|
||
| ```js |
There was a problem hiding this comment.
These should use jsx, not js, for better syntax highlighting.
There was a problem hiding this comment.
I've been told js gives better highlighting than jsx, even for JSX content.
There was a problem hiding this comment.
Oh? I usually see lots of blocks with red backgrounds when using js on JSX code. Here's an example.
js: https://github.com/Khan/aphrodite/tree/436325f8848db974e6f9ffba18afa6af1c426095#api
jsx: https://github.com/Khan/aphrodite/tree/a1fdb520fd9fcc50148a30de2c49c7c1253a604e#api
There was a problem hiding this comment.
That does look better :-) either way's fine, but i'm pretty sure it's come up before. Maybe Github improved the highlighting since.
|
@benstepp You've done a really great job with your first rule! 🎊 |
| if (isTagName(node.parent.name.name) && node.name.name === 'children') { | ||
| context.report({ | ||
| node: node, | ||
| message: 'Do not pass children as props.' |
There was a problem hiding this comment.
It might be nice to additionally tell people what they should do. How about this error message?
Do not pass children as a prop. Instead, nest children between the opening and closing tags.
benstepp
force-pushed
the
bs-no-children-prop
branch
from
3f1d0b2
to
38f051f
Compare
|
I went ahead and made changes to the error messages, and added the additional clarity regarding the desired behavior to the introduction of the docs.
|
| if (childrenProp) { | ||
| context.report({ | ||
| node: node, | ||
| message: 'Do not pass children as props. Instead, pass them as the third argument to React.createElement.' |
There was a problem hiding this comment.
I believe that React.createElement can take an arbitrary number of arguments. Starting with the third argument they are considered children. This error message should probably be updated to reflect this.
benstepp
force-pushed
the
bs-no-children-prop
branch
from
38f051f
to
6fdcc93
Compare
|
Went ahead and added test cases with non-children props and multiple children, as well as clarified the error regarding |
|
LGTM |
benstepp
force-pushed
the
bs-no-children-prop
branch
from
6fdcc93
to
b1f9f08
Compare
|
@benstepp It looks like this needs a rebase. |
benstepp
force-pushed
the
bs-no-children-prop
branch
from
b1f9f08
to
3f1d0b2
Compare
|
@benstepp any chance you can rebase this? |
benstepp
force-pushed
the
bs-no-children-prop
branch
2 times, most recently
from
87cf23a
to
6dc4837
Compare
|
Forgot I still had this over here. Rebased over current master. Same 13 failing tests as master that would probably be better fixed as a separate commit. It is currently marked as recommended, but I think that should be removed until a major version. Let me know what you think. |
|
Agreed on all accounts! Thanks for rebasing. 💃 |
benstepp
force-pushed
the
bs-no-children-prop
branch
from
6dc4837
to
ca68ea9
Compare
| @@ -96,6 +97,7 @@ module.exports = { | |||
| 'react/jsx-no-undef': 2, | |||
| 'react/jsx-uses-react': 2, | |||
| 'react/jsx-uses-vars': 2, | |||
| 'react/no-danger': 2, | |||
There was a problem hiding this comment.
I think you unintentionally added this line, which is causing specs to fail.
There was a problem hiding this comment.
Looks like you're right. Went ahead and removed that line.
Prevents children being passed as props. Children should be actual children. --- * Add Component to tests for no-children-prop * Add note in docs about recommended behavior * Change syntax in docs for no-children-prop to jsx --- * Clarify that multiple arguments may be passed as children to React.createElement * Reorganize tests to show JSX with createElement counterpart immediately following. * Added test cases with a non-children property * Added test cases with multiple children --- * Add no-children-prop to recommended config --- * Remove recommended config for no-children-prop
benstepp
force-pushed
the
bs-no-children-prop
branch
from
ca68ea9
to
52e3303
Compare
|
Thanks! |
Prevents children being passed as props. Children should be actual children.
I've never written an eslint rule, so any suggestions are definitely appreciated.