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

feat(rules): add jsx-props-no-spread-multi #3724

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

SimonSchick
Copy link

@SimonSchick SimonSchick commented Apr 3, 2024

Please see notes in code.

I think this rule has value in catching some niche bugs, I personally only saw this ~3-4 times throughout my career.

Copy link

codecov bot commented Apr 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.75%. Comparing base (a944aa5) to head (57bb4f3).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3724      +/-   ##
==========================================
+ Coverage   94.47%   97.75%   +3.28%     
==========================================
  Files         134      135       +1     
  Lines        9613     9630      +17     
  Branches     3486     3490       +4     
==========================================
+ Hits         9082     9414     +332     
+ Misses        531      216     -315     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

configs/recommended.js Outdated Show resolved Hide resolved
docs/rules/jsx-props-no-spread-multi.md Outdated Show resolved Hide resolved
lib/rules/jsx-props-no-spread-multi.js Outdated Show resolved Hide resolved
lib/types.d.ts Outdated Show resolved Hide resolved
lib/rules/jsx-props-no-spread-multi.js Show resolved Hide resolved
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.

Adding a new rule has a cost, and if you’ve only seen it 3-4 times (i never have), i don’t think it’ll be that useful. (Generally it’s better to discuss a new rule in an issue first so it doesn’t feel like work is wasted in the event it’s not merged)

@SimonSchick
Copy link
Author

No hard feelings if it's not merged, I understand it's more of a niche thing, worst case I will port it over to my own plugin :)

@SimonSchick
Copy link
Author

@ljharb do you have any suggestions where I could sample this rule against a larger set of JSX code in the wild? Github search hasn't been particularly helpful due to the lack of ast node search.

I ran this rule again some internal code bases and found 2 more violations which is still not statistically significant but an indicator that this may be more wide-spread than I thought.

@SimonSchick
Copy link
Author

Any interest/thoughts on this? If not I will close this PR eow and move into a separate library.

@ljharb
Copy link
Member

ljharb commented Apr 22, 2024

@SimonSchick we use https://www.npmjs.com/package/eslint-remote-tester to test things weekly, that might be worth looking into?

@ljharb ljharb force-pushed the feat/jsx-props-no-spread-multi branch from d366ac0 to e0f2872 Compare June 1, 2024 05:37
@ljharb ljharb marked this pull request as draft June 1, 2024 05:38
@SimonSchick SimonSchick marked this pull request as ready for review June 7, 2024 16:48
@SimonSchick
Copy link
Author

Updated, but CI seems to be wonky, I rebased ontop of upstream (this repo).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants