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

fix(lint/useJsxKeyInIterable): don't trigger useJsxKeyInIterable when iterating on non-jsx things #2667

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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),
dyc3 marked this conversation as resolved.
Show resolved Hide resolved
}

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
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
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)}</>
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)}</>

```