Skip to content

Commit

Permalink
fix(linter/react_perf): allow new objects, array, fns, etc in top sco…
Browse files Browse the repository at this point in the history
…pe (#4395)

Consider the following code:
```tsx
import { FC } from 'react'
import { SvgIcon } from '@mui/material'

const StyledIcon = <SvgIcon sx={{ padding: 1, color: '#ff0000' }} />
//          reported violation  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
export const MyComponent: FC = () => {
    return (
        <div>
            {StyledIcon}
            {/* ... */}
        </div>
    )
}
```

This should not be a violation since the JSX is pre-computed and re-used, which
does not break React's `Object.is()` checks.
  • Loading branch information
DonIsaac committed Jul 23, 2024
1 parent 00c7b7d commit ac08de8
Show file tree
Hide file tree
Showing 9 changed files with 147 additions and 104 deletions.
16 changes: 13 additions & 3 deletions crates/oxc_linter/src/rules/react_perf/jsx_no_jsx_as_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ declare_oxc_lint!(

impl Rule for JsxNoJsxAsProp {
fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
if node.scope_id() == ctx.scopes().root_scope_id() {
return;
}
if let AstKind::JSXElement(jsx_elem) = node.kind() {
check_jsx_element(jsx_elem, ctx);
}
Expand Down Expand Up @@ -81,14 +84,21 @@ fn check_expression(expr: &Expression) -> Option<Span> {
fn test() {
use crate::tester::Tester;

let pass = vec![r"<Item callback={this.props.jsx} />"];

let fail = vec![
let pass = vec![
r"<Item callback={this.props.jsx} />",
r"const Foo = () => <Item callback={this.props.jsx} />",
r"<Item jsx={<SubItem />} />",
r"<Item jsx={this.props.jsx || <SubItem />} />",
r"<Item jsx={this.props.jsx ? this.props.jsx : <SubItem />} />",
r"<Item jsx={this.props.jsx || (this.props.component ? this.props.component : <SubItem />)} />",
];

let fail = vec![
r"const Foo = () => (<Item jsx={<SubItem />} />)",
r"const Foo = () => (<Item jsx={this.props.jsx || <SubItem />} />)",
r"const Foo = () => (<Item jsx={this.props.jsx ? this.props.jsx : <SubItem />} />)",
r"const Foo = () => (<Item jsx={this.props.jsx || (this.props.component ? this.props.component : <SubItem />)} />)",
];

Tester::new(JsxNoJsxAsProp::NAME, pass, fail).with_react_perf_plugin(true).test_and_snapshot();
}
18 changes: 15 additions & 3 deletions crates/oxc_linter/src/rules/react_perf/jsx_no_new_array_as_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@ declare_oxc_lint!(

impl Rule for JsxNoNewArrayAsProp {
fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
if node.scope_id() == ctx.scopes().root_scope_id() {
return;
}
if let AstKind::JSXElement(jsx_elem) = node.kind() {
check_jsx_element(jsx_elem, ctx);
}
Expand Down Expand Up @@ -103,9 +106,9 @@ fn check_expression(expr: &Expression) -> Option<Span> {
fn test() {
use crate::tester::Tester;

let pass = vec![r"<Item list={this.props.list} />"];

let fail = vec![
let pass = vec![
r"<Item list={this.props.list} />",
r"const Foo = () => <Item list={this.props.list} />",
r"<Item list={[]} />",
r"<Item list={new Array()} />",
r"<Item list={Array()} />",
Expand All @@ -114,6 +117,15 @@ fn test() {
r"<Item list={this.props.list || (this.props.arr ? this.props.arr : [])} />",
];

let fail = vec![
r"const Foo = () => (<Item list={[]} />)",
r"const Foo = () => (<Item list={new Array()} />)",
r"const Foo = () => (<Item list={Array()} />)",
r"const Foo = () => (<Item list={this.props.list || []} />)",
r"const Foo = () => (<Item list={this.props.list ? this.props.list : []} />)",
r"const Foo = () => (<Item list={this.props.list || (this.props.arr ? this.props.arr : [])} />)",
];

Tester::new(JsxNoNewArrayAsProp::NAME, pass, fail)
.with_react_perf_plugin(true)
.test_and_snapshot();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ declare_oxc_lint!(

impl Rule for JsxNoNewFunctionAsProp {
fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
if node.scope_id() == ctx.scopes().root_scope_id() {
return;
}
if let AstKind::JSXElement(jsx_elem) = node.kind() {
check_jsx_element(jsx_elem, ctx);
}
Expand Down Expand Up @@ -106,22 +109,19 @@ fn test() {
use crate::tester::Tester;

let pass = vec![
r"<Item callback={this.props.callback} />",
r"<Item promise={new Promise()} />",
r"<Item onClick={bind(foo)} />",
r"<Item prop={0} />",
r"var a;<Item prop={a} />",
r"var a;a = 1;<Item prop={a} />",
r"var a;<Item prop={a} />",
r"const Foo = () => <Item callback={this.props.callback} />",
r"const Foo = () => (<Item promise={new Promise()} />)",
r"const Foo = () => (<Item onClick={bind(foo)} />)",
r"const Foo = () => (<Item prop={0} />)",
r"const Foo = () => { var a; return <Item prop={a} /> }",
r"const Foo = () => { var a;a = 1; return <Item prop={a} /> }",
r"const Foo = () => { var a;<Item prop={a} /> }",
r"function foo ({prop1 = function(){}, prop2}) {
return <Comp prop={prop2} />
}",
r"function foo ({prop1, prop2 = function(){}}) {
return <Comp prop={prop1} />
}",
];

let fail = vec![
r"<Item prop={function(){return true}} />",
r"<Item prop={() => true} />",
r"<Item prop={new Function('a', 'alert(a)')}/>",
Expand All @@ -133,6 +133,18 @@ fn test() {
r"<Item prop={this.props.callback || (this.props.cb ? this.props.cb : function(){})} />",
];

let fail = vec![
r"const Foo = () => (<Item prop={function(){return true}} />)",
r"const Foo = () => (<Item prop={() => true} />)",
r"const Foo = () => (<Item prop={new Function('a', 'alert(a)')}/>)",
r"const Foo = () => (<Item prop={Function()}/>)",
r"const Foo = () => (<Item onClick={this.clickHandler.bind(this)} />)",
r"const Foo = () => (<Item callback={this.props.callback || function() {}} />)",
r"const Foo = () => (<Item callback={this.props.callback ? this.props.callback : function() {}} />)",
r"const Foo = () => (<Item prop={this.props.callback || this.props.callback ? this.props.callback : function(){}} />)",
r"const Foo = () => (<Item prop={this.props.callback || (this.props.cb ? this.props.cb : function(){})} />)",
];

Tester::new(JsxNoNewFunctionAsProp::NAME, pass, fail)
.with_react_perf_plugin(true)
.test_and_snapshot();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ declare_oxc_lint!(

impl Rule for JsxNoNewObjectAsProp {
fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
if node.scope_id() == ctx.scopes().root_scope_id() {
return;
}
if let AstKind::JSXElement(jsx_elem) = node.kind() {
check_jsx_element(jsx_elem, ctx);
}
Expand Down Expand Up @@ -102,16 +105,20 @@ fn check_expression(expr: &Expression) -> Option<Span> {
fn test() {
use crate::tester::Tester;

let pass = vec![r"<Item config={staticConfig} />"];
let pass = vec![
r"<Item config={staticConfig} />",
r"<Item config={{}} />",
r"const Foo = () => <Item config={staticConfig} />",
];

let fail = vec![
r"<Item config={{}} />",
r"<Item config={new Object()} />",
r"<Item config={Object()} />",
r"<div style={{display: 'none'}} />",
r"<Item config={this.props.config || {}} />",
r"<Item config={this.props.config ? this.props.config : {}} />",
r"<Item config={this.props.config || (this.props.default ? this.props.default : {})} />",
r"const Foo = () => <Item config={{}} />",
r"const Foo = () => (<Item config={new Object()} />)",
r"const Foo = () => (<Item config={Object()} />)",
r"const Foo = () => (<div style={{display: 'none'}} />)",
r"const Foo = () => (<Item config={this.props.config || {}} />)",
r"const Foo = () => (<Item config={this.props.config ? this.props.config : {}} />)",
r"const Foo = () => (<Item config={this.props.config || (this.props.default ? this.props.default : {})} />)",
];

Tester::new(JsxNoNewObjectAsProp::NAME, pass, fail)
Expand Down
24 changes: 12 additions & 12 deletions crates/oxc_linter/src/snapshots/jsx_no_jsx_as_prop.snap
Original file line number Diff line number Diff line change
Expand Up @@ -2,29 +2,29 @@
source: crates/oxc_linter/src/tester.rs
---
eslint-plugin-react-perf(jsx-no-jsx-as-prop): JSX attribute values should not contain other JSX.
╭─[jsx_no_jsx_as_prop.tsx:1:12]
1<Item jsx={<SubItem />} />
· ───────────
╭─[jsx_no_jsx_as_prop.tsx:1:31]
1const Foo = () => (<Item jsx={<SubItem />} />)
· ───────────
╰────
help: simplify props or memoize props in the parent component (https://react.dev/reference/react/memo#my-component-rerenders-when-a-prop-is-an-object-or-array).

eslint-plugin-react-perf(jsx-no-jsx-as-prop): JSX attribute values should not contain other JSX.
╭─[jsx_no_jsx_as_prop.tsx:1:30]
1<Item jsx={this.props.jsx || <SubItem />} />
· ───────────
╭─[jsx_no_jsx_as_prop.tsx:1:49]
1const Foo = () => (<Item jsx={this.props.jsx || <SubItem />} />)
· ───────────
╰────
help: simplify props or memoize props in the parent component (https://react.dev/reference/react/memo#my-component-rerenders-when-a-prop-is-an-object-or-array).

eslint-plugin-react-perf(jsx-no-jsx-as-prop): JSX attribute values should not contain other JSX.
╭─[jsx_no_jsx_as_prop.tsx:1:46]
1<Item jsx={this.props.jsx ? this.props.jsx : <SubItem />} />
· ───────────
╭─[jsx_no_jsx_as_prop.tsx:1:65]
1const Foo = () => (<Item jsx={this.props.jsx ? this.props.jsx : <SubItem />} />)
· ───────────
╰────
help: simplify props or memoize props in the parent component (https://react.dev/reference/react/memo#my-component-rerenders-when-a-prop-is-an-object-or-array).

eslint-plugin-react-perf(jsx-no-jsx-as-prop): JSX attribute values should not contain other JSX.
╭─[jsx_no_jsx_as_prop.tsx:1:77]
1<Item jsx={this.props.jsx || (this.props.component ? this.props.component : <SubItem />)} />
· ───────────
╭─[jsx_no_jsx_as_prop.tsx:1:96]
1const Foo = () => (<Item jsx={this.props.jsx || (this.props.component ? this.props.component : <SubItem />)} />)
· ───────────
╰────
help: simplify props or memoize props in the parent component (https://react.dev/reference/react/memo#my-component-rerenders-when-a-prop-is-an-object-or-array).
36 changes: 18 additions & 18 deletions crates/oxc_linter/src/snapshots/jsx_no_new_array_as_prop.snap
Original file line number Diff line number Diff line change
Expand Up @@ -2,43 +2,43 @@
source: crates/oxc_linter/src/tester.rs
---
eslint-plugin-react-perf(jsx-no-new-array-as-prop): JSX attribute values should not contain Arrays created in the same scope.
╭─[jsx_no_new_array_as_prop.tsx:1:13]
1<Item list={[]} />
· ──
╭─[jsx_no_new_array_as_prop.tsx:1:32]
1const Foo = () => (<Item list={[]} />)
· ──
╰────
help: simplify props or memoize props in the parent component (https://react.dev/reference/react/memo#my-component-rerenders-when-a-prop-is-an-object-or-array).

eslint-plugin-react-perf(jsx-no-new-array-as-prop): JSX attribute values should not contain Arrays created in the same scope.
╭─[jsx_no_new_array_as_prop.tsx:1:13]
1<Item list={new Array()} />
· ───────────
╭─[jsx_no_new_array_as_prop.tsx:1:32]
1const Foo = () => (<Item list={new Array()} />)
· ───────────
╰────
help: simplify props or memoize props in the parent component (https://react.dev/reference/react/memo#my-component-rerenders-when-a-prop-is-an-object-or-array).

eslint-plugin-react-perf(jsx-no-new-array-as-prop): JSX attribute values should not contain Arrays created in the same scope.
╭─[jsx_no_new_array_as_prop.tsx:1:13]
1<Item list={Array()} />
· ───────
╭─[jsx_no_new_array_as_prop.tsx:1:32]
1const Foo = () => (<Item list={Array()} />)
· ───────
╰────
help: simplify props or memoize props in the parent component (https://react.dev/reference/react/memo#my-component-rerenders-when-a-prop-is-an-object-or-array).

eslint-plugin-react-perf(jsx-no-new-array-as-prop): JSX attribute values should not contain Arrays created in the same scope.
╭─[jsx_no_new_array_as_prop.tsx:1:32]
1<Item list={this.props.list || []} />
· ──
╭─[jsx_no_new_array_as_prop.tsx:1:51]
1const Foo = () => (<Item list={this.props.list || []} />)
· ──
╰────
help: simplify props or memoize props in the parent component (https://react.dev/reference/react/memo#my-component-rerenders-when-a-prop-is-an-object-or-array).

eslint-plugin-react-perf(jsx-no-new-array-as-prop): JSX attribute values should not contain Arrays created in the same scope.
╭─[jsx_no_new_array_as_prop.tsx:1:49]
1<Item list={this.props.list ? this.props.list : []} />
· ──
╭─[jsx_no_new_array_as_prop.tsx:1:68]
1const Foo = () => (<Item list={this.props.list ? this.props.list : []} />)
· ──
╰────
help: simplify props or memoize props in the parent component (https://react.dev/reference/react/memo#my-component-rerenders-when-a-prop-is-an-object-or-array).

eslint-plugin-react-perf(jsx-no-new-array-as-prop): JSX attribute values should not contain Arrays created in the same scope.
╭─[jsx_no_new_array_as_prop.tsx:1:67]
1<Item list={this.props.list || (this.props.arr ? this.props.arr : [])} />
· ──
╭─[jsx_no_new_array_as_prop.tsx:1:86]
1const Foo = () => (<Item list={this.props.list || (this.props.arr ? this.props.arr : [])} />)
· ──
╰────
help: simplify props or memoize props in the parent component (https://react.dev/reference/react/memo#my-component-rerenders-when-a-prop-is-an-object-or-array).
Loading

0 comments on commit ac08de8

Please sign in to comment.