Skip to content

Commit

Permalink
fix(lint/useJsxKeyInIterable): don't trigger useJsxKeyInIterable wh…
Browse files Browse the repository at this point in the history 
…en iterating on non-jsx things (#2667)
  • Loading branch information
@dyc3
dyc3 authored May 3, 2024
1 parent 2a22c97 commit 1b63093
Show file tree
Hide file tree
Showing 4 changed files with 38 additions and 110 deletions.
76 changes: 24 additions & 52 deletions crates/biome_js_analyze/src/lint/correctness/use_jsx_key_in_iterable.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,15 +52,9 @@ declare_node_union! {
pub ReactComponentExpression = JsxTagExpression | JsCallExpression
}

#[derive(Debug)]
pub enum UseJsxKeyInIterableState {
MissingKeyProps(TextRange),
CantDetermineJSXProp(TextRange),
}

impl Rule for UseJsxKeyInIterable {
type Query = Semantic<UseJsxKeyInIterableQuery>;
type State = UseJsxKeyInIterableState;
type State = TextRange;
type Signals = Vec<Self::State>;
type Options = ();

Expand All @@ -77,38 +71,19 @@ impl Rule for UseJsxKeyInIterable {
}

fn diagnostic(_: &RuleContext<Self>, state: &Self::State) -> Option<RuleDiagnostic> {
match state {
UseJsxKeyInIterableState::MissingKeyProps(state) => {
let diagnostic = RuleDiagnostic::new(
rule_category!(),
state,
markup! {
"Missing "<Emphasis>"key"</Emphasis>" property for this element in iterable."
},
)
.note(markup! {
"The order of the items may change, and having a key can help React identify which item was moved."
}).note(markup! {
"Check the "<Hyperlink href="https://react.dev/learn/rendering-lists#why-does-react-need-keys">"React documentation"</Hyperlink>". "
});
Some(diagnostic)
}
UseJsxKeyInIterableState::CantDetermineJSXProp(state) => {
let diagnostic = RuleDiagnostic::new(
rule_category!(),
state,
markup! {
"Cannot determine whether this child has the required "<Emphasis>"key"</Emphasis>" prop."
},
)
.note(markup! {
"Either return a JSX expression, or suppress this instance if you determine it is safe."
}).note(markup! {
"Check the "<Hyperlink href="https://react.dev/learn/rendering-lists#why-does-react-need-keys">"React documentation for why a key prop is required"</Hyperlink>". "
});
Some(diagnostic)
}
}
let diagnostic = RuleDiagnostic::new(
rule_category!(),
state,
markup! {
"Missing "<Emphasis>"key"</Emphasis>" property for this element in iterable."
},
)
.note(markup! {
"The order of the items may change, and having a key can help React identify which item was moved."
}).note(markup! {
"Check the "<Hyperlink href="https://react.dev/learn/rendering-lists#why-does-react-need-keys">"React documentation"</Hyperlink>". "
});
Some(diagnostic)
}
}

Expand All @@ -119,10 +94,7 @@ impl Rule for UseJsxKeyInIterable {
/// ```js
/// [<h1></h1>, <h1></h1>]
/// ```
fn handle_collections(
node: &JsArrayExpression,
model: &SemanticModel,
) -> Vec<UseJsxKeyInIterableState> {
fn handle_collections(node: &JsArrayExpression, model: &SemanticModel) -> Vec<TextRange> {
let is_inside_jsx = node.parent::<JsxExpressionChild>().is_some();
node.elements()
.iter()
Expand All @@ -133,6 +105,7 @@ fn handle_collections(
let node = AnyJsExpression::cast(node.into_syntax())?;
handle_potential_react_component(node, model, is_inside_jsx)
})
.flatten()
.collect()
}

Expand All @@ -143,10 +116,7 @@ fn handle_collections(
/// ```js
/// data.map(x => <h1>{x}</h1>)
/// ```
fn handle_iterators(
node: &JsCallExpression,
model: &SemanticModel,
) -> Option<Vec<UseJsxKeyInIterableState>> {
fn handle_iterators(node: &JsCallExpression, model: &SemanticModel) -> Option<Vec<TextRange>> {
let callee = node.callee().ok()?;
let member_expression = AnyJsMemberExpression::cast(callee.into_syntax())?;
let arguments = node.arguments().ok()?;
Expand Down Expand Up @@ -198,6 +168,7 @@ fn handle_iterators(
let returned_value = statement.argument()?;
handle_potential_react_component(returned_value, model, is_inside_jsx)
})
.flatten()
.collect::<Vec<_>>();

Some(res)
Expand All @@ -207,7 +178,6 @@ fn handle_iterators(
match body {
AnyJsFunctionBody::AnyJsExpression(expr) => {
handle_potential_react_component(expr, model, is_inside_jsx)
.map(|state| vec![state])
}
AnyJsFunctionBody::JsFunctionBody(body) => {
let res = body
Expand All @@ -218,6 +188,7 @@ fn handle_iterators(
let returned_value = statement.argument()?;
handle_potential_react_component(returned_value, model, is_inside_jsx)
})
.flatten()
.collect::<Vec<_>>();
Some(res)
}
Expand All @@ -231,19 +202,20 @@ fn handle_potential_react_component(
node: AnyJsExpression,
model: &SemanticModel,
is_inside_jsx: bool,
) -> Option<UseJsxKeyInIterableState> {
) -> Option<Vec<TextRange>> {
let node = unwrap_parenthesis(node)?;

if is_inside_jsx {
if let Some(node) = ReactComponentExpression::cast_ref(node.syntax()) {
let range = handle_react_component(node, model)?;
Some(UseJsxKeyInIterableState::MissingKeyProps(range))
Some(vec![range])
} else {
Some(UseJsxKeyInIterableState::CantDetermineJSXProp(node.range()))
None
}
} else {
let range =
handle_react_component(ReactComponentExpression::cast_ref(node.syntax())?, model)?;
Some(UseJsxKeyInIterableState::MissingKeyProps(range))
Some(vec![range])
}
}

Expand Down
57 changes: 0 additions & 57 deletions crates/biome_js_analyze/tests/specs/correctness/useJsxKeyInIterable/invalid.jsx.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -520,44 +520,6 @@ invalid.jsx:33:8 lint/correctness/useJsxKeyInIterable ━━━━━━━━
i Check the React documentation.
```

```
invalid.jsx:33:19 lint/correctness/useJsxKeyInIterable ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! Cannot determine whether this child has the required key prop.
31 │ (<h1>{[<h1></h1>, <h1></h1>, <h1></h1>]}</h1>)
32 │
> 33 │ (<h1>{[<h1></h1>, xyz, abc? <h2></h2>: bcd]}</h1>)
│ ^^^
34 │
35 │ (<h1>{data.map(c => <h1></h1>)}</h1>)
i Either return a JSX expression, or suppress this instance if you determine it is safe.
i Check the React documentation for why a key prop is required.
```

```
invalid.jsx:33:24 lint/correctness/useJsxKeyInIterable ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! Cannot determine whether this child has the required key prop.
31 │ (<h1>{[<h1></h1>, <h1></h1>, <h1></h1>]}</h1>)
32 │
> 33 │ (<h1>{[<h1></h1>, xyz, abc? <h2></h2>: bcd]}</h1>)
│ ^^^^^^^^^^^^^^^^^^^
34 │
35 │ (<h1>{data.map(c => <h1></h1>)}</h1>)
i Either return a JSX expression, or suppress this instance if you determine it is safe.
i Check the React documentation for why a key prop is required.
```

```
Expand All @@ -577,25 +539,6 @@ invalid.jsx:35:21 lint/correctness/useJsxKeyInIterable ━━━━━━━━
i Check the React documentation.
```

```
invalid.jsx:37:21 lint/correctness/useJsxKeyInIterable ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! Cannot determine whether this child has the required key prop.
35 │ (<h1>{data.map(c => <h1></h1>)}</h1>)
36 │
> 37 │ (<h1>{data.map(c => xyz)}</h1>)
│ ^^^
38 │
39 │ (<h1>{data.map(c => (<h1></h1>))}</h1>)
i Either return a JSX expression, or suppress this instance if you determine it is safe.
i Check the React documentation for why a key prop is required.
```

```
Expand Down
8 changes: 7 additions & 1 deletion crates/biome_js_analyze/tests/specs/correctness/useJsxKeyInIterable/valid.jsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,4 +45,10 @@ React.Children.map(c => React.cloneElement(c, {key: c}));

(<h1>{data.map(c => (<h1 key={c}></h1>))}</h1>)

(<h1>{data.map(c => {return (<h1 key={c}></h1>)})}</h1>)
(<h1>{data.map(c => {return (<h1 key={c}></h1>)})}</h1>)

<>{data.reduce((total, next) => total + next, 0)}</>

<>{data.reduce((a, b) => Math.max(a, b), 0)}</>

<>{data.reduce((a, b) => a > b ? a : b, 0)}</>
7 changes: 7 additions & 0 deletions crates/biome_js_analyze/tests/specs/correctness/useJsxKeyInIterable/valid.jsx.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,4 +52,11 @@ React.Children.map(c => React.cloneElement(c, {key: c}));
(<h1>{data.map(c => (<h1 key={c}></h1>))}</h1>)

(<h1>{data.map(c => {return (<h1 key={c}></h1>)})}</h1>)

<>{data.reduce((total, next) => total + next, 0)}</>

<>{data.reduce((a, b) => Math.max(a, b), 0)}</>

<>{data.reduce((a, b) => a > b ? a : b, 0)}</>

```

0 comments on commit 1b63093

Please sign in to comment.