eslint-plugin-react
eslint-plugin-react copied to clipboard
Add new rule "react/no-referential-type-default-prop"
Discussed in
https://github.com/yannickcr/eslint-plugin-react/issues/2847
Test
npm run lint
npm run test
shows no errors and warnings
- [x] Added it in README
cc @ljharb
This new rule looked useful so I quickly tested it against +150 repositories. It seems to be crashing quite often on isReactComponentName method. This test should cover at least one of those cases
export default function() {
return <div />;
}
I'm not sure if all crashes were caused by the same issue. You can find all results from here and log containing all crashing cases from here.
@cyan33 ping :-) there's a few things to address
Sorry it took me a bit to get back to this 🙂
Updated:
- Tweaking the name and docs of the rule
- Adding support to check for
Symbol('foo') - Add duplicated test case for default parser,
babel-eslint, andtypescript-eslint. - Add test case for anonymous function
- Simply skip as it's hard to guarantee if it's a real React component just based on its content. I think some false positive is fine in this case since it's not encouraged to not provide a name to the component?
Ping @ljharb :)
Is there anything more to be done before this rule can be release? It looks very useful. cc @ljharb
@fengkx the review comment can be addressed either by @cyan33, or, by someone commenting a link to their branch (NOT another PR) that addresses them.
@ljharb https://github.com/fengkx/eslint-plugin-react/tree/pr-2848-continue I rebased @cyan33 's branch to master branch and make chage according to the review comment.
@fengkx thanks! this PR has been freshly rebased, and I updated the test conventions as well. The only missing piece here is to use Components.detect. Please ping me when your branch link is updated and I'll be able to pull it in and rereview.
Codecov Report
Merging #2848 (4cfe5d2) into master (bf59919) will increase coverage by
0.00%. The diff coverage is97.56%.
@@ Coverage Diff @@
## master #2848 +/- ##
=======================================
Coverage 97.54% 97.54%
=======================================
Files 127 128 +1
Lines 9066 9107 +41
Branches 3307 3320 +13
=======================================
+ Hits 8843 8883 +40
- Misses 223 224 +1
| Impacted Files | Coverage Δ | |
|---|---|---|
| configs/all.js | 100.00% <ø> (ø) |
|
| lib/rules/no-object-type-as-default-prop.js | 97.56% <97.56%> (ø) |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
This branch switched the component detection to use Components.detect. Because the Component.detect function no only check the function has a leading upper case letter but also return value( to be jsx or null), I change all test cases to return a null.
I can't find any doc about the Components.detect function. Should we mention this kind of utils in CONTRIBUTING.md ?
ping @ljharb
Sure, it couldn't hurt to mention it in CONTRIBUTING.md.
Thanks, the added commit looks good; i'll pull it in shortly.
Could you release a new version for this? @ljharb
When I’m prepared to field bug reports from the unreleased changes, i will.