feat(rules): add jsx-props-no-spread-multi #3724
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3724 +/- ##
=======================================
Coverage 97.76% 97.76%
=======================================
Files 133 134 +1
Lines 9467 9485 +18
Branches 3467 3470 +3
=======================================
+ Hits 9255 9273 +18
Misses 212 212 ☔ View full report in Codecov by Sentry. |
| Examples of **incorrect** code for this rule: | ||
|
|
||
| ```jsx | ||
| <App {...props} myAttr='1' {...props} /> |
There was a problem hiding this comment.
TODO: Should I add more expression examples eg. function calls?
| const hashes = new Set(); | ||
| for (const spread of spreads) { | ||
| // TODO: Deep compare ast function? | ||
| const hash = JSON.stringify(spread, propertyFilter); |
There was a problem hiding this comment.
This is admittedly somewhat of a bandaid.
I used a various of deepEqual before to compare all possible matches (which is also O(n^2)).
This performs pretty well and generally has been generating small 'hashes' however I am worried of scenarios where developers create excessively large/syntactically complex spreads.
In these cases we could perform a simple md5/sha hashing though this may be overkill.
There was a problem hiding this comment.
Optimized it a little by only hashing the argument rather than the full node.
|
|
||
| type TypeDeclarationBuilder = (annotation: ASTNode, parentName: string, seen: Set<typeof annotation>) => object; | ||
| type TypeDeclarationBuilder = ( |
There was a problem hiding this comment.
This was auto-formatted, I assume this is ok?
| return { | ||
| JSXOpeningElement(node) { | ||
| const spreads = node.attributes.filter((attr) => attr.type === 'JSXSpreadAttribute'); | ||
| if (spreads.length < 2) { |
There was a problem hiding this comment.
Would be nice to have some stats on how many spreads exists on average per component, though this probably covers ~99.99% of all component instantiations.
|
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 :) |
|
@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. |
|
Any interest/thoughts on this? If not I will close this PR eow and move into a separate library. |
|
@SimonSchick we use https://www.npmjs.com/package/eslint-remote-tester to test things weekly, that might be worth looking into? |
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.