Skip to content

Commit

Permalink
fix(linter/react): Fixed false positive with missing key inside React…
Browse files Browse the repository at this point in the history
….Children.toArray() (#4945)

Closes: #4421 

Added three new functions in order to check if an element is inside of
React.Children.toArray.
Tests added for this fix are exactly the same as those used by
eslint-plugin-react.
  • Loading branch information
vHugoObject committed Aug 22, 2024
1 parent ee7ac8b commit ddf83ff
Show file tree
Hide file tree
Showing 2 changed files with 132 additions and 1 deletion.
102 changes: 101 additions & 1 deletion crates/oxc_linter/src/rules/react/jsx_key.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use oxc_ast::{
ast::{JSXAttributeItem, JSXAttributeName, JSXElement, JSXFragment, Statement},
ast::{Expression, JSXAttributeItem, JSXAttributeName, JSXElement, JSXFragment, Statement},
AstKind,
};
use oxc_diagnostics::OxcDiagnostic;
Expand Down Expand Up @@ -69,6 +69,76 @@ impl Rule for JsxKey {
}
}

pub fn is_to_array(node: &AstNode<'_>) -> bool {
const TOARRAY: &str = "toArray";

let AstKind::CallExpression(call) = node.kind() else { return false };

let Some(subject) = call.callee_name() else { return false };

if subject != TOARRAY {
return false;
}

true
}

pub fn import_matcher<'a>(
ctx: &LintContext<'a>,
actual_local_name: &'a str,
expected_module_name: &'a str,
) -> bool {
ctx.semantic().module_record().import_entries.iter().any(|import| {
import.module_request.name().as_str() == expected_module_name.to_lowercase()
&& import.local_name.name().as_str() == actual_local_name
})
}

pub fn is_import<'a>(
ctx: &LintContext<'a>,
actual_local_name: &'a str,
expected_local_name: &'a str,
expected_module_name: &'a str,
) -> bool {
let total_imports = ctx.semantic().module_record().requested_modules.len();
let total_variables = ctx.scopes().get_bindings(ctx.scopes().root_scope_id()).len();

if total_variables == 0 && total_imports == 0 {
return actual_local_name == expected_local_name;
}

import_matcher(ctx, actual_local_name, expected_module_name)
}

pub fn is_children<'a, 'b>(node: &'b AstNode<'a>, ctx: &'b LintContext<'a>) -> bool {
const REACT: &str = "React";
const CHILDREN: &str = "Children";

let AstKind::CallExpression(call) = node.kind() else { return false };

let Some(member) = call.callee.as_member_expression() else { return false };

if let Expression::Identifier(ident) = member.object() {
return is_import(ctx, ident.name.as_str(), CHILDREN, REACT);
}

let Some(inner_member) = member.object().get_inner_expression().as_member_expression() else {
return false;
};

let Some(ident) = inner_member.object().get_identifier_reference() else { return false };

let Some(local_name) = inner_member.static_property_name() else { return false };

return is_import(ctx, ident.name.as_str(), REACT, REACT) && local_name == CHILDREN;
}
fn is_within_children_to_array<'a, 'b>(node: &'b AstNode<'a>, ctx: &'b LintContext<'a>) -> bool {
let parents_iter = ctx.nodes().iter_parents(node.id()).skip(2);
parents_iter
.filter(|parent_node| matches!(parent_node.kind(), AstKind::CallExpression(_)))
.any(|parent_node| is_children(parent_node, ctx) && is_to_array(parent_node))
}

enum InsideArrayOrIterator {
Array,
Iterator(Span),
Expand Down Expand Up @@ -151,6 +221,9 @@ fn is_in_array_or_iter<'a, 'b>(

fn check_jsx_element<'a>(node: &AstNode<'a>, jsx_elem: &JSXElement<'a>, ctx: &LintContext<'a>) {
if let Some(outer) = is_in_array_or_iter(node, ctx) {
if is_within_children_to_array(node, ctx) {
return;
}
if !jsx_elem.opening_element.attributes.iter().any(|attr| {
let JSXAttributeItem::Attribute(attr) = attr else {
return false;
Expand Down Expand Up @@ -400,6 +473,20 @@ fn test() {
}
];
",
r"{React.Children.toArray(items.map((item) => {
return (
<div>
{item}
</div>
);}))}
",
r#"import { Children } from "react";
Children.toArray([1, 2 ,3].map(x => <App />));
"#,
r#"import React from "react";
React.Children.toArray([1, 2 ,3].map(x => <App />));
"#,
r"React.Children.toArray([1, 2 ,3].map(x => <App />));",
];

let fail = vec![
Expand Down Expand Up @@ -500,6 +587,19 @@ fn test() {
);
};
",
r"foo.Children.toArray([1, 2 ,3].map(x => <App />));",
r"
import Act from 'react';
import { Children as ReactChildren } from 'react';
const { Children } = Act;
const { toArray } = Children;
Act.Children.toArray([1, 2 ,3].map(x => <App />));
Act.Children.toArray(Array.from([1, 2 ,3], x => <App />));
Children.toArray([1, 2 ,3].map(x => <App />));
Children.toArray(Array.from([1, 2 ,3], x => <App />));
",
];

Tester::new(JsxKey::NAME, pass, fail).test_and_snapshot();
Expand Down
31 changes: 31 additions & 0 deletions crates/oxc_linter/src/snapshots/jsx_key.snap
Original file line number Diff line number Diff line change
Expand Up @@ -315,3 +315,34 @@ source: crates/oxc_linter/src/tester.rs
8 │ <Text foo bar baz qux onClick={() => onClickHandler()} onPointerDown={() => onPointerDownHandler()} onMouseDown={() => onMouseDownHandler()} />
╰────
help: Add a "key" prop to the element in the iterator (https://react.dev/learn/rendering-lists#keeping-list-items-in-order-with-key).

⚠ eslint-plugin-react(jsx-key): Missing "key" prop for element in iterator.
╭─[jsx_key.tsx:1:32]
1 │ foo.Children.toArray([1, 2 ,3].map(x => <App />));
· ─┬─ ─┬─
· │ ╰── Element generated here.
· ╰── Iterator starts here.
╰────
help: Add a "key" prop to the element in the iterator (https://react.dev/learn/rendering-lists#keeping-list-items-in-order-with-key).

⚠ eslint-plugin-react(jsx-key): Missing "key" prop for element in iterator.
╭─[jsx_key.tsx:10:36]
9 │ Act.Children.toArray(Array.from([1, 2 ,3], x => <App />));
10 │ Children.toArray([1, 2 ,3].map(x => <App />));
· ─┬─ ─┬─
· │ ╰── Element generated here.
· ╰── Iterator starts here.
11 │ Children.toArray(Array.from([1, 2 ,3], x => <App />));
╰────
help: Add a "key" prop to the element in the iterator (https://react.dev/learn/rendering-lists#keeping-list-items-in-order-with-key).

⚠ eslint-plugin-react(jsx-key): Missing "key" prop for element in iterator.
╭─[jsx_key.tsx:11:32]
10 │ Children.toArray([1, 2 ,3].map(x => <App />));
11 │ Children.toArray(Array.from([1, 2 ,3], x => <App />));
· ──┬─ ─┬─
· │ ╰── Element generated here.
· ╰── Iterator starts here.
12 │
╰────
help: Add a "key" prop to the element in the iterator (https://react.dev/learn/rendering-lists#keeping-list-items-in-order-with-key).

0 comments on commit ddf83ff

Please sign in to comment.