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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more question, should CommonJS also work?
const { Children } = require('react');
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.
Yes, definitely.
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.
ping @silvenon, are you interested in completing this PR?
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 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-key
rule should always succeed if we're insideReact.Children.toArray()
because omittingkey
there doesn't cause a React warning.Fixes #1574.