Skip to content

Commit a0dc6b9

Browse files
morgan-codedmorgan-coded
committed
jsx-no-leaked-render: preserve expression alternates
1 parent c9a2de7 commit a0dc6b9

2 files changed

Lines changed: 225 additions & 9 deletions

File tree

‎lib/rules/jsx-no-leaked-render.js‎

Lines changed: 45 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,14 +7,17 @@
77

88
const find = require('es-iterator-helpers/Iterator.prototype.find');
99
const from = require('es-iterator-helpers/Iterator.from');
10+
const arrayIncludes = require('array-includes');
1011

11-
const getText = require('../util/eslint').getText;
12+
const eslintUtil = require('../util/eslint');
1213
const docsUrl = require('../util/docsUrl');
1314
const report = require('../util/report');
1415
const variableUtil = require('../util/variable');
1516
const testReactVersion = require('../util/version').testReactVersion;
1617
const isParenthesized = require('../util/ast').isParenthesized;
1718

19+
const getText = eslintUtil.getText;
20+
1821
//------------------------------------------------------------------------------
1922
// Rule Definition
2023
//------------------------------------------------------------------------------
@@ -27,7 +30,39 @@ const COERCE_STRATEGY = 'coerce';
2730
const TERNARY_STRATEGY = 'ternary';
2831
const DEFAULT_VALID_STRATEGIES = [TERNARY_STRATEGY, COERCE_STRATEGY];
2932
const COERCE_VALID_LEFT_SIDE_EXPRESSIONS = ['UnaryExpression', 'BinaryExpression', 'CallExpression'];
30-
const TERNARY_INVALID_ALTERNATE_VALUES = [undefined, null, false];
33+
const TERNARY_INVALID_ALTERNATE_VALUES = [null, false];
34+
35+
function isGlobalUndefined(context, node) {
36+
const variable = variableUtil.getVariableFromContext(context, node, node.name);
37+
return !variable || !variable.defs || variable.defs.length === 0;
38+
}
39+
40+
function isInvalidTernaryAlternate(context, node) {
41+
if (node.type === 'Identifier') {
42+
return node.name === 'undefined' && isGlobalUndefined(context, node);
43+
}
44+
if (node.type === 'UnaryExpression') {
45+
return node.operator === 'void' && node.argument.type === 'Literal';
46+
}
47+
return node.type === 'Literal' && arrayIncludes(TERNARY_INVALID_ALTERNATE_VALUES, node.value);
48+
}
49+
50+
function isFalseLiteral(node) {
51+
return node.type === 'Literal' && node.value === false;
52+
}
53+
54+
function getCoerceAlternateText(context, node) {
55+
const nodeText = getText(context, node);
56+
if (
57+
node.type === 'AssignmentExpression'
58+
|| node.type === 'ConditionalExpression'
59+
|| node.type === 'LogicalExpression'
60+
|| node.type === 'SequenceExpression'
61+
) {
62+
return `(${nodeText})`;
63+
}
64+
return nodeText;
65+
}
3166

3267
function trimLeftNode(node) {
3368
// Remove double unary expression (boolean coercion), so we avoid trimming valid negations
@@ -80,19 +115,22 @@ function ruleFixer(context, fixStrategy, fixer, reportedNode, leftNode, rightNod
80115
if (isParenthesized(context, node)) {
81116
nodeText = `(${nodeText})`;
82117
}
83-
if (node.parent && node.parent.type === 'ConditionalExpression' && node.parent.consequent.value === false) {
118+
if (node.parent && node.parent.type === 'ConditionalExpression' && isFalseLiteral(node.parent.consequent)) {
84119
return `${getIsCoerceValidNestedLogicalExpression(node) ? '' : '!'}${nodeText}`;
85120
}
86121
return `${getIsCoerceValidNestedLogicalExpression(node) ? '' : '!!'}${nodeText}`;
87122
}).join(' && ');
88123

89-
if (rightNode.parent && rightNode.parent.type === 'ConditionalExpression' && rightNode.parent.consequent.value === false) {
124+
if (rightNode.parent && rightNode.parent.type === 'ConditionalExpression' && isFalseLiteral(rightNode.parent.consequent)) {
90125
const consequentVal = rightNode.parent.consequent.raw || rightNode.parent.consequent.name;
91-
const alternateVal = rightNode.parent.alternate.raw || rightNode.parent.alternate.name;
126+
const alternateVal = getCoerceAlternateText(context, rightNode.parent.alternate);
92127
if (rightNode.parent.test && rightNode.parent.test.type === 'LogicalExpression') {
93128
return fixer.replaceText(reportedNode, `${newText} ? ${consequentVal} : ${alternateVal}`);
94129
}
95-
return fixer.replaceText(reportedNode, `${newText} && ${alternateVal}`);
130+
return fixer.replaceText(
131+
reportedNode,
132+
`${newText} && ${alternateVal}`
133+
);
96134
}
97135

98136
if (rightNode.type === 'ConditionalExpression' || rightNode.type === 'LogicalExpression') {
@@ -211,9 +249,7 @@ module.exports = {
211249
return;
212250
}
213251

214-
const isValidTernaryAlternate = TERNARY_INVALID_ALTERNATE_VALUES.indexOf(node.alternate.value) === -1;
215-
const isJSXElementAlternate = node.alternate.type === 'JSXElement';
216-
if (isValidTernaryAlternate || isJSXElementAlternate) {
252+
if (!isInvalidTernaryAlternate(context, node.alternate) && !isFalseLiteral(node.consequent)) {
217253
return;
218254
}
219255

‎tests/lib/rules/jsx-no-leaked-render.js‎

Lines changed: 180 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,14 @@ ruleTester.run('jsx-no-leaked-render', rule, {
126126
`,
127127
options: [{ validStrategies: ['coerce', 'ternary'] }],
128128
},
129+
{
130+
code: `
131+
function Component({ count, title }, undefined) {
132+
return <div>{count ? title : undefined}</div>
133+
}
134+
`,
135+
options: [{ validStrategies: ['coerce'] }],
136+
},
129137

130138
// Fixes for:
131139
// - https://github.com/jsx-eslint/eslint-plugin-react/issues/3292
@@ -169,6 +177,34 @@ ruleTester.run('jsx-no-leaked-render', rule, {
169177
`,
170178
options: [{ validStrategies: ['coerce'] }],
171179
},
180+
{
181+
// It shouldn't delete valid alternate branches from ternary expressions when "coerce" is the only valid strategy
182+
code: `
183+
const Component = ({ filteredOptions, selectVal, options }) => {
184+
return (
185+
<Select
186+
options={
187+
filteredOptions.length
188+
? groupedOptions(filteredOptions)
189+
: selectVal
190+
? []
191+
: groupedOptions(options)
192+
}
193+
/>
194+
)
195+
}
196+
`,
197+
options: [{ validStrategies: ['coerce'] }],
198+
},
199+
{
200+
// It shouldn't drop side effects from ternary alternate branches when "coerce" is the only valid strategy
201+
code: `
202+
const Component = ({ count, title }) => {
203+
return <div>{count ? title : void sideEffect()}</div>
204+
}
205+
`,
206+
options: [{ validStrategies: ['coerce'] }],
207+
},
172208
// Fixes for:
173209
// - https://github.com/jsx-eslint/eslint-plugin-react/issues/3354
174210
{
@@ -791,6 +827,42 @@ ruleTester.run('jsx-no-leaked-render', rule, {
791827
}
792828
`,
793829
},
830+
{
831+
code: `
832+
const Component = ({ count, title }) => {
833+
return <div>{count ? title : undefined}</div>
834+
}
835+
`,
836+
options: [{ validStrategies: ['coerce'] }],
837+
errors: [{
838+
message: 'Potential leaked value that might cause unintentionally rendered values or rendering crashes',
839+
line: 3,
840+
column: 24,
841+
}],
842+
output: `
843+
const Component = ({ count, title }) => {
844+
return <div>{!!count && title}</div>
845+
}
846+
`,
847+
},
848+
{
849+
code: `
850+
const Component = ({ count, title }) => {
851+
return <div>{count ? title : void 0}</div>
852+
}
853+
`,
854+
options: [{ validStrategies: ['coerce'] }],
855+
errors: [{
856+
message: 'Potential leaked value that might cause unintentionally rendered values or rendering crashes',
857+
line: 3,
858+
column: 24,
859+
}],
860+
output: `
861+
const Component = ({ count, title }) => {
862+
return <div>{!!count && title}</div>
863+
}
864+
`,
865+
},
794866
{
795867
code: `
796868
const Component = ({ count, title }) => {
@@ -924,6 +996,114 @@ ruleTester.run('jsx-no-leaked-render', rule, {
924996
column: 38,
925997
}],
926998
},
999+
semver.satisfies(eslintPkg.version, '> 4') ? {
1000+
code: `
1001+
const MyComponent = ({ Empty }) => {
1002+
return <div>{isIndeterminate ? false : <Empty />}</div>
1003+
}
1004+
`,
1005+
output: `
1006+
const MyComponent = ({ Empty }) => {
1007+
return <div>{!isIndeterminate && <Empty />}</div>
1008+
}
1009+
`,
1010+
options: [{ validStrategies: ['coerce'] }],
1011+
errors: [{
1012+
message: 'Potential leaked value that might cause unintentionally rendered values or rendering crashes',
1013+
line: 3,
1014+
column: 24,
1015+
}],
1016+
} : [],
1017+
semver.satisfies(eslintPkg.version, '> 4') ? {
1018+
code: `
1019+
const MyComponent = () => {
1020+
return <div>{isIndeterminate ? false : getContent()}</div>
1021+
}
1022+
`,
1023+
output: `
1024+
const MyComponent = () => {
1025+
return <div>{!isIndeterminate && getContent()}</div>
1026+
}
1027+
`,
1028+
options: [{ validStrategies: ['coerce'] }],
1029+
errors: [{
1030+
message: 'Potential leaked value that might cause unintentionally rendered values or rendering crashes',
1031+
line: 3,
1032+
column: 24,
1033+
}],
1034+
} : [],
1035+
semver.satisfies(eslintPkg.version, '> 4') ? {
1036+
code: `
1037+
const MyComponent = ({ isFoo }) => {
1038+
return <div>{isIndeterminate ? false : isFoo ? <Aaa /> : <Bbb />}</div>
1039+
}
1040+
`,
1041+
output: `
1042+
const MyComponent = ({ isFoo }) => {
1043+
return <div>{!isIndeterminate && (isFoo ? <Aaa /> : <Bbb />)}</div>
1044+
}
1045+
`,
1046+
options: [{ validStrategies: ['coerce'] }],
1047+
errors: [{
1048+
message: 'Potential leaked value that might cause unintentionally rendered values or rendering crashes',
1049+
line: 3,
1050+
column: 24,
1051+
}],
1052+
} : [],
1053+
semver.satisfies(eslintPkg.version, '> 4') ? {
1054+
code: `
1055+
const MyComponent = ({ fallback }) => {
1056+
return <div>{isIndeterminate ? false : fallback || <Empty />}</div>
1057+
}
1058+
`,
1059+
output: `
1060+
const MyComponent = ({ fallback }) => {
1061+
return <div>{!isIndeterminate && (fallback || <Empty />)}</div>
1062+
}
1063+
`,
1064+
options: [{ validStrategies: ['coerce'] }],
1065+
errors: [{
1066+
message: 'Potential leaked value that might cause unintentionally rendered values or rendering crashes',
1067+
line: 3,
1068+
column: 24,
1069+
}],
1070+
} : [],
1071+
semver.satisfies(eslintPkg.version, '> 4') ? {
1072+
code: `
1073+
const MyComponent = ({ fallback }) => {
1074+
return <div>{ready && isIndeterminate ? false : fallback || <Empty />}</div>
1075+
}
1076+
`,
1077+
output: `
1078+
const MyComponent = ({ fallback }) => {
1079+
return <div>{!!ready && !!isIndeterminate ? false : (fallback || <Empty />)}</div>
1080+
}
1081+
`,
1082+
options: [{ validStrategies: ['coerce'] }],
1083+
errors: [{
1084+
message: 'Potential leaked value that might cause unintentionally rendered values or rendering crashes',
1085+
line: 3,
1086+
column: 24,
1087+
}],
1088+
} : [],
1089+
semver.satisfies(eslintPkg.version, '> 4') ? {
1090+
code: `
1091+
const MyComponent = () => {
1092+
return <div>{ready && isIndeterminate ? false : (track(), <Empty />)}</div>
1093+
}
1094+
`,
1095+
output: `
1096+
const MyComponent = () => {
1097+
return <div>{!!ready && !!isIndeterminate ? false : (track(), <Empty />)}</div>
1098+
}
1099+
`,
1100+
options: [{ validStrategies: ['coerce'] }],
1101+
errors: [{
1102+
message: 'Potential leaked value that might cause unintentionally rendered values or rendering crashes',
1103+
line: 3,
1104+
column: 24,
1105+
}],
1106+
} : [],
9271107
{
9281108
code: `
9291109
const MyComponent = () => {

0 commit comments

Comments
 (0)