Skip to content
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

[New] add no-object-type-as-default-prop rule #2848

Merged

Conversation

cyan33
Copy link
Contributor

@cyan33 cyan33 commented Oct 30, 2020

Discussed in

#2847

Test

npm run lint
npm run test

shows no errors and warnings


  • Added it in README

Co-authored-by: cyan33 <[email protected]>
Co-authored-by: fengkx <[email protected]>
Co-authored-by: Jordan Harband <[email protected]>
@cyan33 cyan33 force-pushed the no-referential-type-default-praram-rule branch from 1adfa98 to 3510760 Compare October 30, 2020 22:42
@cyan33
Copy link
Contributor Author

cyan33 commented Oct 30, 2020

cc @ljharb

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM minus the naming/prose concerns, the question about new Symbols, and the test parser comment

docs/rules/no-reference-type-as-default-prop.md Outdated Show resolved Hide resolved
docs/rules/no-reference-type-as-default-prop.md Outdated Show resolved Hide resolved
docs/rules/no-reference-type-as-default-prop.md Outdated Show resolved Hide resolved
tests/lib/rules/no-reference-type-as-default-prop.js Outdated Show resolved Hide resolved
@AriPerkkio
Copy link
Contributor

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.

@ljharb
Copy link
Member

ljharb commented Nov 6, 2020

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

@cyan33 cyan33 force-pushed the no-referential-type-default-praram-rule branch from 3510760 to e17848e Compare November 9, 2020 00:08
@cyan33
Copy link
Contributor Author

cyan33 commented Nov 9, 2020

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 cyan33 force-pushed the no-referential-type-default-praram-rule branch from e17848e to d9e3627 Compare November 9, 2020 00:21
@cyan33 cyan33 requested a review from ljharb November 10, 2020 17:40
@cyan33
Copy link
Contributor Author

cyan33 commented Nov 16, 2020

Ping @ljharb :)

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I think this looks good. After this pass, if my only feedback is docs prose, I'll tweak it myself prior to landing.

docs/rules/no-object-type-as-default-prop.md Outdated Show resolved Hide resolved
docs/rules/no-object-type-as-default-prop.md Outdated Show resolved Hide resolved
docs/rules/no-object-type-as-default-prop.md Outdated Show resolved Hide resolved
docs/rules/no-object-type-as-default-prop.md Outdated Show resolved Hide resolved
docs/rules/no-object-type-as-default-prop.md Outdated Show resolved Hide resolved
docs/rules/no-object-type-as-default-prop.md Outdated Show resolved Hide resolved
lib/rules/no-object-type-as-default-prop.js Outdated Show resolved Hide resolved
lib/rules/no-object-type-as-default-prop.js Outdated Show resolved Hide resolved
lib/rules/no-object-type-as-default-prop.js Outdated Show resolved Hide resolved
lib/rules/no-object-type-as-default-prop.js Outdated Show resolved Hide resolved
@fengkx
Copy link
Contributor

fengkx commented Oct 11, 2022

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

@ljharb
Copy link
Member

ljharb commented Oct 11, 2022

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

@fengkx
Copy link
Contributor

fengkx commented Oct 11, 2022

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

@ljharb ljharb force-pushed the no-referential-type-default-praram-rule branch from d9e3627 to d66ea00 Compare October 14, 2022 21:46
@ljharb ljharb changed the title Add new rule "react/no-referential-type-default-prop" [New] add no-object-type-as-default-prop rule Oct 14, 2022
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This rule should use the Components.detect utility rather than trying to implement its own component detection.

@ljharb
Copy link
Member

ljharb commented Oct 14, 2022

@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
Copy link

codecov bot commented Oct 14, 2022

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.

@fengkx
Copy link
Contributor

fengkx commented Oct 15, 2022

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

@ljharb
Copy link
Member

ljharb commented Oct 15, 2022

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

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

@ljharb ljharb force-pushed the no-referential-type-default-praram-rule branch from d66ea00 to 4cfe5d2 Compare October 15, 2022 16:19
@ljharb ljharb marked this pull request as ready for review October 15, 2022 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

4 participants