feat(js_biome_analyze): implement noDuplicatedSpreadProps #8116
Conversation
🦋 Changeset detectedLatest commit: 92df86e The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
A-CLI
|
Caution Review failedThe pull request is closed. WalkthroughAdds a new nursery lint rule Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (9)
📒 Files selected for processing (6)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/biome_js_analyze/tests/specs/nursery/noDuplicateSpread/valid.jsx (1)
1-12: Valid fixtures cover basics; consider a couple of extra shapesThe three cases nicely exercise “single spread” and “multiple different spreads”. If you feel like padding coverage, you could add:
- A non–self-closing element (
<div>...</div>) to hit theJsxOpeningElementbranch explicitly.- A case mixing identifier and non-identifier spreads to ensure they peacefully coexist once the core logic is adjusted.
Not blocking, just belt-and-braces.
crates/biome_js_analyze/src/lint/nursery/no_duplicate_spread.rs (1)
89-101: Diagnostic text is clear; tiny wording nit (optional)“The expression X has spread more than once” is understandable, but “has been spread more than once” might read more naturally. Totally optional, the current wording is perfectly usable.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
crates/biome_cli/src/execute/migrate/eslint_any_rule_to_biome.rsis excluded by!**/migrate/eslint_any_rule_to_biome.rsand included by**crates/biome_configuration/src/analyzer/linter/rules.rsis excluded by!**/rules.rsand included by**crates/biome_configuration/src/generated/domain_selector.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_diagnostics_categories/src/categories.rsis excluded by!**/categories.rsand included by**crates/biome_js_analyze/src/lint/nursery.rsis excluded by!**/nursery.rsand included by**crates/biome_js_analyze/tests/specs/nursery/noDuplicateSpread/invalid.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noDuplicateSpread/valid.jsx.snapis excluded by!**/*.snapand included by**packages/@biomejs/backend-jsonrpc/src/workspace.tsis excluded by!**/backend-jsonrpc/src/workspace.tsand included by**packages/@biomejs/biome/configuration_schema.jsonis excluded by!**/configuration_schema.jsonand included by**
📒 Files selected for processing (6)
.changeset/clean-swans-act.md(1 hunks)crates/biome_js_analyze/src/lint/nursery/no_duplicate_spread.rs(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noDuplicateSpread/invalid.jsx(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noDuplicateSpread/valid.jsx(1 hunks)crates/biome_rule_options/src/lib.rs(1 hunks)crates/biome_rule_options/src/no_duplicate_spread.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
crates/biome_js_analyze/src/lint/nursery/no_duplicate_spread.rs (1)
crates/biome_analyze/src/rule.rs (4)
recommended(602-605)sources(617-620)same(246-251)domains(632-635)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Bench (biome_configuration)
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: Check JS Files
- GitHub Check: autofix
- GitHub Check: Test Node.js API
- GitHub Check: Documentation
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: End-to-end tests
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Check Dependencies
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_js_parser)
🔇 Additional comments (5)
.changeset/clean-swans-act.md (1)
1-11: Changeset description matches the implemented ruleText and example line up with
noDuplicateSpread’s current behaviour (blocking duplicate identifier spreads). Nothing to tweak here.crates/biome_rule_options/src/lib.rs (1)
66-66: Options module wiring looks goodThe
no_duplicate_spreadmodule is exported consistently with the rest of the rule options; no issues from my side.crates/biome_js_analyze/tests/specs/nursery/noDuplicateSpread/invalid.jsx (1)
1-3: Invalid fixture is focused and fine as-isThis is a nice minimal repro for the duplicate identifier case; the free
propsidentifier is acceptable for a syntactic lint fixture.crates/biome_rule_options/src/no_duplicate_spread.rs (1)
1-6: Options container matches existing patterns
NoDuplicateSpreadOptionsmirrors other empty options structs (derives, serde attrs, optional schema), so this should plug into the config story cleanly.If you want extra peace of mind, run the existing rule-options tests (or codegen checks) to ensure the new module is picked up everywhere.
crates/biome_js_analyze/src/lint/nursery/no_duplicate_spread.rs (1)
11-40: Double‑check alignment with ESLint’sjsx-props-no-spread-multiThis rule only flags duplicate spreads of the same identifier, whereas the original ESLint rule’s name suggests “more than one spread at all”. You also classify it as
same()logic insources.If the intent is to be a narrower variant (“no duplicate spreads”) rather than a strict port, it might be worth:
- Confirming against the upstream rule docs; and
- Either adjusting the description /
sourcesclassification, or clarifying in the docs that this is intentionally more permissive.Just want to avoid surprising users who come in expecting ESLint‑identical behaviour.
crates/biome_js_analyze/src/lint/nursery/no_duplicated_spread_props.rs
Outdated
Show resolved
Hide resolved
CodSpeed Performance ReportMerging #8116 will not alter performanceComparing Summary
Footnotes
|
|
This rule should be called
EDIT: Alternatively, consider if this function should be added to the Solid domain too. Some other suggestions: |
Yup, I think adding to the solid domain is valid, both jsx. Would go for the non-react name |
Netail
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/biome_js_analyze/src/lint/nursery/no_duplicated_spread_props.rs (1)
48-66: Fix the logic to skip non-identifier spreads instead of abandoning the entire check.This is the same critical bug identified in the previous review. The
?operators on lines 55-57 cause early return when a non-identifier spread is encountered (e.g.,{...foo.bar}), preventing detection of subsequent duplicate identifier spreads.Please apply the fix suggested in the previous review comment, which replaces the
?chain with explicit pattern matching andcontinuestatements. Also add the regression test case toinvalid.jsxas requested earlier in this review.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
crates/biome_cli/src/execute/migrate/eslint_any_rule_to_biome.rsis excluded by!**/migrate/eslint_any_rule_to_biome.rsand included by**crates/biome_configuration/src/analyzer/linter/rules.rsis excluded by!**/rules.rsand included by**crates/biome_configuration/src/generated/domain_selector.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_diagnostics_categories/src/categories.rsis excluded by!**/categories.rsand included by**crates/biome_js_analyze/src/lint/nursery.rsis excluded by!**/nursery.rsand included by**crates/biome_js_analyze/tests/specs/nursery/noDuplicatedSpreadProps/invalid.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noDuplicatedSpreadProps/valid.jsx.snapis excluded by!**/*.snapand included by**packages/@biomejs/backend-jsonrpc/src/workspace.tsis excluded by!**/backend-jsonrpc/src/workspace.tsand included by**packages/@biomejs/biome/configuration_schema.jsonis excluded by!**/configuration_schema.jsonand included by**
📒 Files selected for processing (6)
.changeset/clean-swans-act.md(1 hunks)crates/biome_js_analyze/src/lint/nursery/no_duplicated_spread_props.rs(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noDuplicatedSpreadProps/invalid.jsx(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noDuplicatedSpreadProps/valid.jsx(1 hunks)crates/biome_rule_options/src/lib.rs(1 hunks)crates/biome_rule_options/src/no_duplicated_spread_props.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/biome_rule_options/src/lib.rs
🧰 Additional context used
🧬 Code graph analysis (1)
crates/biome_js_analyze/src/lint/nursery/no_duplicated_spread_props.rs (1)
crates/biome_analyze/src/rule.rs (4)
recommended(602-605)sources(617-620)same(246-251)domains(632-635)
🔇 Additional comments (7)
crates/biome_js_analyze/tests/specs/nursery/noDuplicatedSpreadProps/valid.jsx (1)
1-12: LGTM! Valid test cases are well-structured.The three test cases properly cover scenarios where duplicate spreads don't occur: single spread with props, different spreads, and proper separation.
.changeset/clean-swans-act.md (1)
5-5: Verify the rule name with the maintainer.The PR comments indicate that @ematipico recommended naming this rule
noReactDuplicateSpreadrather thannoDuplicatedSpreadProps, arguing that domain-specific rules should include the domain name to avoid confusion. Please confirm which name should be used before finalising the changeset.crates/biome_rule_options/src/no_duplicated_spread_props.rs (1)
1-6: LGTM! Options struct is properly scaffolded.Empty options struct with all the right derives and serde attributes for future extensibility.
crates/biome_js_analyze/src/lint/nursery/no_duplicated_spread_props.rs (4)
1-10: LGTM! Imports are appropriate.All necessary dependencies are present for the rule implementation.
32-38: Confirm the rule name with maintainer feedback.The PR discussion indicates @ematipico recommended
noReactDuplicateSpreadto clearly indicate this is React-specific and avoid confusion with JavaScript spread. The current namenoDuplicatedSpreadPropsdoesn't include the domain prefix. Please confirm which naming convention to follow.On line 38, you've correctly included both
RuleDomain::ReactandRuleDomain::Solidas discussed.
42-46: LGTM! Node union covers both JSX element types.Properly handles both opening and self-closing elements.
68-104: LGTM! Rule implementation is clean.The trait implementation correctly handles both JSX element types, and the diagnostic message clearly explains the issue and suggests a fix. The logic is sound once the
validate_attributesfunction is corrected.
crates/biome_js_analyze/tests/specs/nursery/noDuplicatedSpreadProps/invalid.jsx
Outdated
Show resolved
Hide resolved
Netail
force-pushed
the
feat/no-duplicate-spread
branch
3 times, most recently
from
040af8d to
a65cf3a
Compare
crates/biome_js_analyze/src/lint/nursery/no_duplicated_spread_props.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/biome_js_analyze/src/lint/nursery/no_duplicated_spread_props.rs (1)
48-66: Critical bug: early return prevents detection of subsequent duplicates.Lines 56-57 use the
?operator, which returnsNonefrom the entire function ifname()orvalue_token()fail. This means if you encounter a malformed spread attribute after valid ones, you'll abandon the entire check instead of continuing to validate remaining attributes.The past review comment correctly identified this issue and suggested using
let-elsewithcontinue, but the fix was never applied.Apply this diff to fix the logic:
fn validate_attributes(list: &JsxAttributeList) -> Option<String> { let mut seen_spreads = HashSet::new(); for attribute in list { - if let AnyJsxAttribute::JsxSpreadAttribute(spread) = attribute - && let Some(argument) = spread.argument().ok() - && let Some(express) = argument.as_js_identifier_expression() - { - let name = express.name().ok()?; - let value_token = name.value_token().ok()?; - let text = value_token.text_trimmed().to_string(); - if !seen_spreads.insert(text.clone()) { - return Some(text); - } + if let AnyJsxAttribute::JsxSpreadAttribute(spread) = attribute { + let Ok(argument) = spread.argument() else { + continue; + }; + + let Some(express) = argument.as_js_identifier_expression() else { + continue; + }; + + let Ok(name) = express.name() else { + continue; + }; + + let Ok(value_token) = name.value_token() else { + continue; + }; + + let text = value_token.text_trimmed().to_string(); + if !seen_spreads.insert(text) { + return Some(text); + } } } None }
🧹 Nitpick comments (1)
crates/biome_js_analyze/src/lint/nursery/no_duplicated_spread_props.rs (1)
59-59: Remove unnecessary clone.The
textvalue isn't used afterinsert, so cloning is wasteful. The diff above removes it.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
crates/biome_cli/src/execute/migrate/eslint_any_rule_to_biome.rsis excluded by!**/migrate/eslint_any_rule_to_biome.rsand included by**crates/biome_configuration/src/analyzer/linter/rules.rsis excluded by!**/rules.rsand included by**crates/biome_configuration/src/generated/domain_selector.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_diagnostics_categories/src/categories.rsis excluded by!**/categories.rsand included by**crates/biome_js_analyze/src/lint/nursery.rsis excluded by!**/nursery.rsand included by**crates/biome_js_analyze/tests/specs/nursery/noDuplicatedSpreadProps/invalid.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noDuplicatedSpreadProps/valid.jsx.snapis excluded by!**/*.snapand included by**packages/@biomejs/backend-jsonrpc/src/workspace.tsis excluded by!**/backend-jsonrpc/src/workspace.tsand included by**packages/@biomejs/biome/configuration_schema.jsonis excluded by!**/configuration_schema.jsonand included by**
📒 Files selected for processing (6)
.changeset/clean-swans-act.md(1 hunks)crates/biome_js_analyze/src/lint/nursery/no_duplicated_spread_props.rs(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noDuplicatedSpreadProps/invalid.jsx(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noDuplicatedSpreadProps/valid.jsx(1 hunks)crates/biome_rule_options/src/lib.rs(1 hunks)crates/biome_rule_options/src/no_duplicated_spread_props.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/biome_js_analyze/tests/specs/nursery/noDuplicatedSpreadProps/valid.jsx
🧰 Additional context used
🧬 Code graph analysis (1)
crates/biome_js_analyze/src/lint/nursery/no_duplicated_spread_props.rs (1)
crates/biome_analyze/src/rule.rs (4)
recommended(602-605)sources(617-620)same(246-251)domains(632-635)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: End-to-end tests
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: Check Dependencies
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Documentation
- GitHub Check: Test Node.js API
- GitHub Check: Check JS Files
- GitHub Check: Bench (biome_configuration)
- GitHub Check: autofix
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: Bench (biome_js_formatter)
🔇 Additional comments (4)
crates/biome_js_analyze/tests/specs/nursery/noDuplicatedSpreadProps/invalid.jsx (1)
1-7: Test coverage looks good.Both test cases appropriately cover duplicate spread detection, including the regression case with non-identifier spreads mixed with duplicates.
.changeset/clean-swans-act.md (1)
1-11: Changeset is clear and well-documented.The description and example appropriately communicate the new rule to users.
crates/biome_rule_options/src/no_duplicated_spread_props.rs (1)
1-6: Options struct is properly configured.The derives and serde attributes are appropriate for a rule options type, even though it's currently empty.
crates/biome_js_analyze/src/lint/nursery/no_duplicated_spread_props.rs (1)
68-103: Rule implementation and diagnostic are well-structured.The trait implementation correctly dispatches to both JSX element types, and the diagnostic message is clear and actionable.
Netail
force-pushed
the
feat/no-duplicate-spread
branch
from
a65cf3a to
7efaf59
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/biome_js_analyze/src/lint/nursery/no_duplicated_spread_props.rs (1)
59-59: Minor: Unnecessary allocationThe
.clone()is unnecessary here. Consider using aHashSet<&str>instead to avoid allocating strings for comparison, or simply usetextdirectly without cloning.- if !seen_spreads.insert(text.clone()) { - return Some(text); + if !seen_spreads.insert(text.as_str()) { + return Some(text); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
crates/biome_cli/src/execute/migrate/eslint_any_rule_to_biome.rsis excluded by!**/migrate/eslint_any_rule_to_biome.rsand included by**crates/biome_configuration/src/analyzer/linter/rules.rsis excluded by!**/rules.rsand included by**crates/biome_configuration/src/generated/domain_selector.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_diagnostics_categories/src/categories.rsis excluded by!**/categories.rsand included by**crates/biome_js_analyze/src/lint/nursery.rsis excluded by!**/nursery.rsand included by**crates/biome_js_analyze/tests/specs/nursery/noDuplicatedSpreadProps/invalid.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noDuplicatedSpreadProps/valid.jsx.snapis excluded by!**/*.snapand included by**packages/@biomejs/backend-jsonrpc/src/workspace.tsis excluded by!**/backend-jsonrpc/src/workspace.tsand included by**packages/@biomejs/biome/configuration_schema.jsonis excluded by!**/configuration_schema.jsonand included by**
📒 Files selected for processing (6)
.changeset/clean-swans-act.md(1 hunks)crates/biome_js_analyze/src/lint/nursery/no_duplicated_spread_props.rs(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noDuplicatedSpreadProps/invalid.jsx(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noDuplicatedSpreadProps/valid.jsx(1 hunks)crates/biome_rule_options/src/lib.rs(1 hunks)crates/biome_rule_options/src/no_duplicated_spread_props.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/biome_rule_options/src/lib.rs
- crates/biome_js_analyze/tests/specs/nursery/noDuplicatedSpreadProps/invalid.jsx
- crates/biome_js_analyze/tests/specs/nursery/noDuplicatedSpreadProps/valid.jsx
- .changeset/clean-swans-act.md
🧰 Additional context used
🧬 Code graph analysis (1)
crates/biome_js_analyze/src/lint/nursery/no_duplicated_spread_props.rs (1)
crates/biome_analyze/src/rule.rs (4)
recommended(602-605)sources(617-620)same(246-251)domains(632-635)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Check JS Files
- GitHub Check: Documentation
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: End-to-end tests
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: Check Dependencies
- GitHub Check: Test Node.js API
- GitHub Check: autofix
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_configuration)
🔇 Additional comments (6)
crates/biome_rule_options/src/no_duplicated_spread_props.rs (1)
1-6: LGTM!Empty options struct follows the standard pattern for rules without configurable options. All necessary derives and attributes are present.
crates/biome_js_analyze/src/lint/nursery/no_duplicated_spread_props.rs (5)
1-10: LGTM!All imports are necessary and correctly organised.
11-40: LGTM!Rule declaration is well-documented with clear examples. The domains (React and Solid) and sources correctly reflect the PR discussion and original eslint-react rule.
42-46: LGTM!Union correctly includes both opening and self-closing JSX elements.
68-87: LGTM!Rule implementation correctly handles both JSX element variants.
89-103: LGTM!Diagnostic message is clear and includes helpful guidance.
crates/biome_js_analyze/src/lint/nursery/no_duplicated_spread_props.rs
Outdated
Show resolved
Hide resolved
| declare_node_union! { | ||
| pub NoDuplicatedSpreadPropsQuery = | ||
| JsxOpeningElement | ||
| | JsxSelfClosingElement | ||
| } | ||
|
|
||
| fn validate_attributes(list: &JsxAttributeList) -> Option<String> { | ||
| let mut seen_spreads = HashSet::new(); | ||
|
|
||
| for attribute in list { | ||
| if let AnyJsxAttribute::JsxSpreadAttribute(spread) = attribute | ||
| && let Some(argument) = spread.argument().ok() | ||
| { | ||
| let express = argument.as_js_identifier_expression()?; | ||
| let name = express.name().ok()?; | ||
| let value_token = name.value_token().ok()?; | ||
| let text = value_token.text_trimmed().to_string(); | ||
| if !seen_spreads.insert(text.clone()) { | ||
| return Some(text); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| None | ||
| } |
There was a problem hiding this comment.
As a general rule, we tend to have the implementation of the rule after the impl Rule trait block
crates/biome_js_analyze/src/lint/nursery/no_duplicated_spread_props.rs
Outdated
Show resolved
Hide resolved
| declare_node_union! { | ||
| pub NoDuplicatedSpreadPropsQuery = | ||
| JsxOpeningElement | ||
| | JsxSelfClosingElement | ||
| } | ||
|
|
||
| fn validate_attributes(list: &JsxAttributeList) -> Option<String> { | ||
| let mut seen_spreads = HashSet::new(); | ||
|
|
||
| for attribute in list { | ||
| if let AnyJsxAttribute::JsxSpreadAttribute(spread) = attribute | ||
| && let Some(argument) = spread.argument().ok() | ||
| && let Some(express) = argument.as_js_identifier_expression() | ||
| { | ||
| let name = express.name().ok()?; | ||
| let value_token = name.value_token().ok()?; | ||
| let text = value_token.text_trimmed().to_string(); | ||
| if !seen_spreads.insert(text.clone()) { | ||
| return Some(text); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| None | ||
| } |
There was a problem hiding this comment.
Can you move this code after impl Rule block?
| /* should not generate diagnostics */ | ||
| const Valid1 = () => { | ||
| return <div {...props} something="else" /> | ||
| } | ||
|
|
||
| const Valid2 = () => { | ||
| return <div something="else" {...props} /> | ||
| } | ||
|
|
||
| const Valid3 = () => { | ||
| return <div {...props} something="else" {...otherProps} /> | ||
| } |
There was a problem hiding this comment.
We aren't testing opening elements. Can you please add more tests? e.g. <p {...s} {...s}></p>
Netail
force-pushed
the
feat/no-duplicate-spread
branch
from
6a3094c to
1820f21
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/biome_js_analyze/tests/specs/nursery/noDuplicatedSpreadProps/valid.jsx (1)
1-32: Valid coverage looks good; optional extra sibling-element caseThese cases nicely cover both
<div></div>and self-closing JSX, with different spread positions and the empty-object spread. If you fancy one more edge case, a component with two sibling elements both spreading the same identifier (each only once) would make it explicit the rule is per-element, not per-component — but what you have is already solid.crates/biome_js_analyze/src/lint/nursery/no_duplicated_spread_props.rs (2)
12-40: Docs and diagnostic read well; a couple of tiny wording nitsThe rule description and example look clear, and the diagnostic is on point. If you want to polish the English a touch:
- Line 14: “Enforces that each unique expression is only spread once.”
- Line 76: “has been spread more than once.”
- Line 80: maybe “…will lead to unnecessary work. Reduce spreads of this expression to one.” reads a bit smoother.
Totally cosmetic, feel free to ignore if you like the current phrasing better.
Also applies to: 69-82
86-103: Attribute scanning logic looks correct; current scope is identifiers-onlyThe
validate_attributesloop is tidy and now correctly skips malformed spreads instead of bailing early, which fixes the earlier?issue. As implemented, duplicates are only detected when the spread argument is a plain identifier expression (e.g.props), and anything more complex (this.props,foo.bar, calls, etc.) is intentionally ignored.That’s a perfectly reasonable conservative first cut; if you ever decide you want parity with more expression shapes, this is the one place to extend. From a correctness perspective for identifier spreads, this looks good to ship.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
crates/biome_js_analyze/tests/specs/nursery/noDuplicatedSpreadProps/invalid.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noDuplicatedSpreadProps/valid.jsx.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (3)
crates/biome_js_analyze/src/lint/nursery/no_duplicated_spread_props.rs(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noDuplicatedSpreadProps/invalid.jsx(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noDuplicatedSpreadProps/valid.jsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/biome_js_analyze/tests/specs/nursery/noDuplicatedSpreadProps/invalid.jsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Documentation
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: Test Node.js API
- GitHub Check: End-to-end tests
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Bench (biome_configuration)
- GitHub Check: Check Dependencies
- GitHub Check: autofix
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: Check JS Files
Netail
force-pushed
the
feat/no-duplicate-spread
branch
from
1820f21 to
96dc7bd
Compare
ematipico
left a comment
ematipico
left a comment
There was a problem hiding this comment.
Awesome! I leave the merging to you 🖖
No computer for the weekend, but in case you guys wanna merge it already, be my guest :) |
dyc3
force-pushed
the
feat/no-duplicate-spread
branch
from
96dc7bd to
92df86e
Compare
3 participants
Summary
Implement Eslint React's
jsx-props-no-spread-multiCloses #7658
Test Plan
Docs