eslint-plugin-react icon indicating copy to clipboard operation
eslint-plugin-react copied to clipboard

Add new rule "react/no-referential-type-default-prop"

Open cyan33 opened this issue 5 years ago • 5 comments

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

cyan33 avatar Oct 30 '20 22:10 cyan33

cc @ljharb

cyan33 avatar Oct 30 '20 22:10 cyan33

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.

AriPerkkio avatar Nov 04 '20 14:11 AriPerkkio

@cyan33 ping :-) there's a few things to address

ljharb avatar Nov 06 '20 22:11 ljharb

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, and typescript-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?

cyan33 avatar Nov 09 '20 00:11 cyan33

Ping @ljharb :)

cyan33 avatar Nov 16 '20 19:11 cyan33

Is there anything more to be done before this rule can be release? It looks very useful. cc @ljharb

fengkx avatar Oct 11 '22 02:10 fengkx

@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 avatar Oct 11 '22 02:10 ljharb

@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 avatar Oct 11 '22 12:10 fengkx

@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.

ljharb avatar Oct 14 '22 21:10 ljharb

Codecov Report

Merging #2848 (4cfe5d2) into master (bf59919) will increase coverage by 0.00%. The diff coverage is 97.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.

codecov[bot] avatar Oct 14 '22 21:10 codecov[bot]

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

fengkx avatar Oct 15 '22 15:10 fengkx

Sure, it couldn't hurt to mention it in CONTRIBUTING.md.

Thanks, the added commit looks good; i'll pull it in shortly.

ljharb avatar Oct 15 '22 16:10 ljharb

Could you release a new version for this? @ljharb

fengkx avatar Oct 28 '22 02:10 fengkx

When I’m prepared to field bug reports from the unreleased changes, i will.

ljharb avatar Oct 28 '22 02:10 ljharb