Handle spread props in no-children-prop rule #862
Conversation
Nodes of type `ExperimentalSpreadProperty` don’t have a `key` property,
causing the `no-children-prop` to raise a “Cannot read property ‘name’
of undefined” error.
Code such as:
```
return React.createElement(
elementType,
{ className: classNames, ...otherProps },
children
)
```
will trigger the error.
It turns out that eslint-plugin-react’s no `no-children-prop` has a bug when dealing with spread props, and that gets triggered in `layout/Base/index.js`. I’ve submitted a PR to that project (jsx-eslint/eslint-plugin-react#862), but for now am pointing at my PR branch. It would be possible to extract the props to a variable before passing them to `React.createElement` to work around the bug. I’m happy to go that way instead of having a github dependency in the project. Or, we could let this PR sit open for a few days to see if the PR gets merged upstream.
| @@ -15,6 +15,7 @@ var RuleTester = require('eslint').RuleTester; | |||
| var parserOptions = { | |||
| ecmaVersion: 6, | |||
| ecmaFeatures: { | |||
| experimentalObjectRestSpread: true, | |||
There was a problem hiding this comment.
let's not necessarily enable this for every test - can we enable it just for the new tests?
There was a problem hiding this comment.
I thought about doing that, but in looking through all of the tests in the project, there isn't a single other case where different parserOptions are used for different test cases. There are eight other test cases that add experimentalObjectRestSpread: true to their top-level parserOptions, though.
That said, if you think it's important to establish a new pattern here, I'm happy to make the change.
There was a problem hiding this comment.
I'm not 100% on it either way - it just seems like it'd be useful to test the parser with common (non-experimental) options.
There was a problem hiding this comment.
I think you've got a valid point.
But I'm pretty big on code consistency, so I wonder if it would be better to do a separate PR with that change made in all of the cases like this.
There was a problem hiding this comment.
I'm not sure why - this is the PR that adds experimentalObjectRestSpread. but i don't think this needs to block if other maintainers think it's fine.
There was a problem hiding this comment.
Yes, this PR adds it here. But there are eight other tests that include experimentalObjectRestSpread in their default parserOptions just like this one does.
If it's important to only set that option for certain test cases, then I think we should make that consistent across all of the tests that use that option. That's why I suggested a separate PR to make that change.
|
Merged, thanks! |
|
@yannickcr any update on a release? |
|
@ljharb sorry, I was pretty busy last days but I'll try to craft a release this week-end. |
Nodes of type
ExperimentalSpreadPropertydon’t have akeyproperty, causing theno-children-propto raise a “Cannot read property ‘name’ of undefined” error.Code such as:
will trigger the error.