Skip to content

Commit

Permalink
[Fix] jsx-no-leaked-render: avoid unnecessary negation operators an…
Browse files Browse the repository at this point in the history
…d ternary branches deletion

Closes jsx-eslint#3297. Relates to jsx-eslint#3292.
  • Loading branch information
Belco90 authored and ljharb committed May 26, 2022
1 parent c42b624 commit e7fc22f
Show file tree
Hide file tree
Showing 3 changed files with 76 additions and 8 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@ This change log adheres to standards from [Keep a CHANGELOG](https://keepachange

### Fixed
* [`display-name`]: fix false positive for HOF returning only nulls ([#3291][] @golopot)
* [`jsx-no-leaked-render`]: avoid unnecessary negation operators and ternary branches deletion ([#3299][] @Belco90)

### Changed
* [Docs] [`jsx-tag-spacing`]: rename option from [#3264][] ([#3294[] @ljharb)
* [Docs] [`jsx-key`]: split the examples ([#3293][] @ioggstream)

[#3299]: https://github.com/jsx-eslint/eslint-plugin-react/pull/3299
[#3294]: https://github.com/jsx-eslint/eslint-plugin-react/issues/3294
[#3293]: https://github.com/jsx-eslint/eslint-plugin-react/pull/3293
[#3291]: https://github.com/jsx-eslint/eslint-plugin-react/pull/3291
Expand Down
25 changes: 20 additions & 5 deletions lib/rules/jsx-no-leaked-render.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ const COERCE_STRATEGY = 'coerce';
const TERNARY_STRATEGY = 'ternary';
const DEFAULT_VALID_STRATEGIES = [TERNARY_STRATEGY, COERCE_STRATEGY];
const COERCE_VALID_LEFT_SIDE_EXPRESSIONS = ['UnaryExpression', 'BinaryExpression', 'CallExpression'];
const TERNARY_INVALID_ALTERNATE_VALUES = [undefined, null, false];

function trimLeftNode(node) {
// Remove double unary expression (boolean coercion), so we avoid trimming valid negations
Expand All @@ -31,6 +32,14 @@ function trimLeftNode(node) {
return node;
}

function getIsCoerceValidNestedLogicalExpression(node) {
if (node.type === 'LogicalExpression') {
return getIsCoerceValidNestedLogicalExpression(node.left) && getIsCoerceValidNestedLogicalExpression(node.right);
}

return COERCE_VALID_LEFT_SIDE_EXPRESSIONS.some((validExpression) => validExpression === node.type);
}

function ruleFixer(context, fixStrategy, fixer, reportedNode, leftNode, rightNode) {
const sourceCode = context.getSourceCode();
const rightSideText = sourceCode.getText(rightNode);
Expand Down Expand Up @@ -102,11 +111,12 @@ module.exports = {
'JSXExpressionContainer > LogicalExpression[operator="&&"]'(node) {
const leftSide = node.left;

if (
validStrategies.has(COERCE_STRATEGY)
&& COERCE_VALID_LEFT_SIDE_EXPRESSIONS.some((validExpression) => validExpression === leftSide.type)
) {
return;
const isCoerceValidLeftSide = COERCE_VALID_LEFT_SIDE_EXPRESSIONS
.some((validExpression) => validExpression === leftSide.type);
if (validStrategies.has(COERCE_STRATEGY)) {
if (isCoerceValidLeftSide || getIsCoerceValidNestedLogicalExpression(leftSide)) {
return;
}
}

report(context, messages.noPotentialLeakedRender, 'noPotentialLeakedRender', {
Expand All @@ -122,6 +132,11 @@ module.exports = {
return;
}

const isValidTernaryAlternate = TERNARY_INVALID_ALTERNATE_VALUES.indexOf(node.alternate.value) === -1;
if (isValidTernaryAlternate) {
return;
}

report(context, messages.noPotentialLeakedRender, 'noPotentialLeakedRender', {
node,
fix(fixer) {
Expand Down
57 changes: 54 additions & 3 deletions tests/lib/rules/jsx-no-leaked-render.js
Original file line number Diff line number Diff line change
Expand Up @@ -93,36 +93,87 @@ ruleTester.run('jsx-no-leaked-render', rule, {
`,
},
{
options: [{ validStrategies: ['ternary'] }],
code: `
const Component = ({ elements, count }) => {
return <div>{count ? <List elements={elements}/> : null}</div>
}
`,
options: [{ validStrategies: ['ternary'] }],
},
{
options: [{ validStrategies: ['coerce'] }],
code: `
const Component = ({ elements, count }) => {
return <div>{!!count && <List elements={elements}/>}</div>
}
`,
options: [{ validStrategies: ['coerce'] }],
},
{
options: [{ validStrategies: ['coerce', 'ternary'] }],
code: `
const Component = ({ elements, count }) => {
return <div>{count ? <List elements={elements}/> : null}</div>
}
`,
options: [{ validStrategies: ['coerce', 'ternary'] }],
},
{
code: `
const Component = ({ elements, count }) => {
return <div>{!!count && <List elements={elements}/>}</div>
}
`,
options: [{ validStrategies: ['coerce', 'ternary'] }],
},
{
code: `
const Component = ({ elements, count }) => {
return <div>{!!count && <List elements={elements}/>}</div>
}
`,
options: [{ validStrategies: ['coerce'] }],
},

// Fixes for:
// - https://github.com/jsx-eslint/eslint-plugin-react/issues/3292
// - https://github.com/jsx-eslint/eslint-plugin-react/issues/3297
{
// It shouldn't delete valid alternate from ternary expressions when "coerce" is the only valid strategy
code: `
const Component = ({ elements, count }) => {
return (
<div>
<div> {direction ? (direction === "down" ? "▼" : "▲") : ""} </div>
<div>{ containerName.length > 0 ? "Loading several stuff" : "Loading" }</div>
</div>
)
}
`,
options: [{ validStrategies: ['coerce'] }],
},
{
// It shouldn't delete valid branches from ternary expressions when ["coerce", "ternary"] are only valid strategies
code: `
const Component = ({ elements, count }) => {
return <div>{direction ? (direction === "down" ? "▼" : "▲") : ""}</div>
}
`,
options: [{ validStrategies: ['coerce', 'ternary'] }],
},
{
// It shouldn't report nested logical expressions when "coerce" is the only valid strategy
code: `
const Component = ({ direction }) => {
return (
<div>
<div>{!!direction && direction === "down" && "▼"}</div>
<div>{direction === "down" && !!direction && "▼"}</div>
<div>{direction === "down" || !!direction && "▼"}</div>
<div>{(!display || display === DISPLAY.WELCOME) && <span>foo</span>}</div>
</div>
)
}
`,
options: [{ validStrategies: ['coerce'] }],
},
]),

Expand Down

0 comments on commit e7fc22f

Please sign in to comment.