Ignore jsx-key if inside React.Children.toArray() #1591
Conversation
jsx-key rule should always succeed if we're inside React.Children.toArray() because omitting the key there doesn't cause a React warning. Fixes jsx-eslint#1574.
tests/lib/rules/jsx-key.js
Outdated
| { | ||
| code: ` | ||
| import { Children } from "react"; | ||
| Children.toArray([1, 2 ,3].map(x => <App />)); |
There was a problem hiding this comment.
Let's also include a test case that has const { toArray } = Children, and another that has something called toArray and Children that aren't imported from React.
There was a problem hiding this comment.
Good idea, my changes wouldn't catch those cases. I'm also worried about other likely cases, this is a test case that I'm currently trying to get working as valid:
import React from 'react';
import { Children as ReactChildren } from 'react';
const { Children } = React;
const { toArray } = Children;
React.Children.toArray([1, 2 ,3].map(x => <App />));
ReactChildren.toArray([1, 2 ,3].map(x => <App />));
Children.toArray([1, 2 ,3].map(x => <App />));
toArray([1, 2 ,3].map(x => <App />));I need to get used to hunting down variable/import declarations. 😄
There was a problem hiding this comment.
One more question, should CommonJS also work?
const { Children } = require('react');There was a problem hiding this comment.
ping @silvenon, are you interested in completing this PR?
There was a problem hiding this comment.
I am, but unfortunately I don't have the skills for it yet 😥
|
I rebased this, and with virtually no modification, the tests (which still fail on master) seem to pass now on this PR! I'll try to take a look at the comment I made above now. |
|
I'm going to go ahead and merge as-is, since it now respects the React pragma, and this is a long-awaited fix. |
Codecov Report
@@ Coverage Diff @@
## master #1591 +/- ##
=======================================
Coverage 97.62% 97.62%
=======================================
Files 123 123
Lines 8962 8974 +12
Branches 3275 3278 +3
=======================================
+ Hits 8749 8761 +12
Misses 213 213
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
Nice, thank you for finishing this! ❤️ |
jsx-keyrule should always succeed if we're insideReact.Children.toArray()because omittingkeythere doesn't cause a React warning.Fixes #1574.