Skip to content

Commit 579aea4

Browse files
NickvanDykeNickvanDyke
committed
Flag derived state regardless of source when single setter call
#17
1 parent 86e085e commit 579aea4

File tree

5 files changed

+87
-70
lines changed

5 files changed

+87
-70
lines changed

‎src/rule.js‎

Lines changed: 36 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,9 @@ import {
1313
isState,
1414
isProp,
1515
isHOCProp,
16+
countStateSetterCalls,
1617
} from "./util/react.js";
18+
import { arraysEqual } from "./util/javascript.js";
1719

1820
export const name = "you-might-not-need-an-effect";
1921

@@ -95,8 +97,8 @@ export const rule = {
9597
});
9698
}
9799

98-
// TODO: Make more readable
99-
const isArgsInternal = callExpr.arguments
100+
// TODO: Make more readable (and performant)
101+
const isAllArgsInternal = callExpr.arguments
100102
.flatMap((arg) => getDownstreamRefs(context, arg))
101103
.flatMap((ref) =>
102104
getUpstreamReactVariables(context, ref.identifier),
@@ -106,7 +108,7 @@ export const rule = {
106108
isState(variable) ||
107109
(isProp(variable) && !isHOCProp(variable)),
108110
);
109-
const isArgsExternal = callExpr.arguments
111+
const isSomeArgsExternal = callExpr.arguments
110112
.flatMap((arg) => getDownstreamRefs(context, arg))
111113
.flatMap((ref) =>
112114
getUpstreamReactVariables(context, ref.identifier),
@@ -116,7 +118,24 @@ export const rule = {
116118
(!isState(variable) && !isProp(variable)) ||
117119
isHOCProp(variable),
118120
);
119-
const isDepsInternal = depsRefs
121+
const isAllArgsInDeps = callExpr.arguments
122+
.flatMap((arg) => getDownstreamRefs(context, arg))
123+
// Need to do this prematurely here because we call notEmptyEvery on the refs,
124+
// not on the upstream variables (which also filters out parameters)
125+
// TODO: Think about how to centralize that.
126+
.filter((ref) =>
127+
ref.resolved.defs.every((def) => def.type !== "Parameter"),
128+
)
129+
.notEmptyEvery((argRef) =>
130+
depsRefs.some((depRef) =>
131+
// If they have the same upstream variables, they're equivalent
132+
arraysEqual(
133+
getUpstreamReactVariables(context, argRef.identifier),
134+
getUpstreamReactVariables(context, depRef.identifier),
135+
),
136+
),
137+
);
138+
const isAllDepsInternal = depsRefs
120139
.flatMap((ref) =>
121140
getUpstreamReactVariables(context, ref.identifier),
122141
)
@@ -126,20 +145,26 @@ export const rule = {
126145
(isProp(variable) && !isHOCProp(variable)),
127146
);
128147

129-
if (isArgsInternal) {
130-
// TODO: Can also warn if this is the only call to the setter,
131-
// even if the arg is external (and not retrieved in the effect)...
132-
// Does it matter whether the args are in the deps array?
133-
// I guess so, to differentiate between derived and chain state updates?
134-
// What about internal vs in deps? Changes behavior, but meaningfully?
148+
if (
149+
isAllArgsInternal ||
150+
// They are always in sync, regardless of source - could compute during render
151+
// TODO: Should we *always* check that the args are in deps?
152+
// Should/could that replace isArgsInternal?
153+
// Should it be chained state when not?
154+
(isAllArgsInDeps && countStateSetterCalls(ref) === 1)
155+
) {
135156
context.report({
136157
node: callExpr,
137158
messageId: messageIds.avoidDerivedState,
138159
data: { state: stateName },
139160
});
140161
}
141162

142-
if (!isArgsInternal && !isArgsExternal && isDepsInternal) {
163+
if (
164+
!isAllArgsInternal &&
165+
!isSomeArgsExternal &&
166+
isAllDepsInternal
167+
) {
143168
context.report({
144169
node: callExpr,
145170
messageId: messageIds.avoidChainingState,

‎src/util/javascript.js‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
Array.prototype.notEmptyEvery = function (predicate) {
2+
return this.length > 0 && this.every(predicate);
3+
};
4+
5+
export const arraysEqual = (arr1, arr2) => {
6+
if (arr1.length !== arr2.length) {
7+
return false;
8+
}
9+
return arr1.every((element, index) => element === arr2[index]);
10+
};

‎src/util/react.js‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -229,7 +229,10 @@ const countUseStates = (context, componentNode) => {
229229
return count;
230230
};
231231

232-
// Returns the component or custom hook that contains the `useEffect` node.
232+
export const countStateSetterCalls = (stateSetterRef) =>
233+
stateSetterRef.resolved.references.length - 1; // -1 for the initial declaration
234+
235+
// Returns the component or custom hook that contains the `useEffect` node
233236
const findContainingNode = (node) => {
234237
if (!node) {
235238
return undefined;
@@ -260,7 +263,3 @@ export const getUpstreamReactVariables = (context, node) =>
260263
isProp(variable) ||
261264
variable.defs.every((def) => def.type !== "Parameter"),
262265
);
263-
264-
Array.prototype.notEmptyEvery = function (predicate) {
265-
return this.length > 0 && this.every(predicate);
266-
};

‎test/deriving-state.test.js‎

Lines changed: 4 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -239,23 +239,6 @@ new MyRuleTester().run("/deriving-state", {
239239
}
240240
`,
241241
},
242-
{
243-
name: "From internal and external state",
244-
code: js`
245-
import { getPrefixFor } from 'library';
246-
import { useState } from 'react';
247-
248-
function Component() {
249-
const [name, setName] = useState();
250-
const [prefixedName, setPrefixedName] = useState();
251-
const prefix = getPrefixFor(name);
252-
253-
useEffect(() => {
254-
setPrefixedName(prefix + name);
255-
}, [name, prefix])
256-
}
257-
`,
258-
},
259242
{
260243
name: "From derived internal and external state",
261244
code: js`
@@ -265,12 +248,12 @@ new MyRuleTester().run("/deriving-state", {
265248
function Component() {
266249
const [name, setName] = useState();
267250
const [prefixedName, setPrefixedName] = useState();
268-
const prefix = getPrefixFor(name);
269-
const newValue = prefix + name;
270251
271252
useEffect(() => {
253+
const prefix = getPrefixFor(name);
254+
const newValue = prefix + name; // Make it a little more interesting
272255
setPrefixedName(newValue);
273-
}, [newValue])
256+
}, [name])
274257
}
275258
`,
276259
},
@@ -462,18 +445,12 @@ new MyRuleTester().run("/deriving-state", {
462445
},
463446
{
464447
name: "From external state with single setter call",
465-
todo: true,
466448
code: js`
467449
function Feed() {
468-
const { data: posts } = useQuery('/posts');
450+
const { data: posts } = fetch('/posts');
469451
const [selectedPost, setSelectedPost] = useState();
470452
471453
useEffect(() => {
472-
// This is the only place that modifies the state,
473-
// thus they will always be in sync and it could be computed during render
474-
// Difficult bit is that a single state setter call is legit when the
475-
// external state is initialized inside the effect (i.e. retrieved from external system)
476-
// Hopefully 'isDirectCall' will mostly catch that now.
477454
setSelectedPost(posts[0]);
478455
}, [posts]);
479456
}
@@ -487,7 +464,6 @@ new MyRuleTester().run("/deriving-state", {
487464
},
488465
{
489466
name: "From derived external state with single setter call",
490-
todo: true,
491467
code: js`
492468
function Form() {
493469
const name = useQuery('/name');

‎test/syntax.test.js‎

Lines changed: 33 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -91,32 +91,6 @@ new MyRuleTester().run("/syntax", {
9191
}
9292
`,
9393
},
94-
{
95-
name: "Destructured array skips element in arrow function params",
96-
code: js`
97-
function FilteredPosts() {
98-
const posts = useSomeAPI();
99-
const [filteredPosts, setFilteredPosts] = useState([]);
100-
101-
useEffect(() => {
102-
// Resulting AST node looks like:
103-
// {
104-
// "type": "ArrayPattern",
105-
// "elements": [
106-
// null, <-- Must handle this!
107-
// {
108-
// "type": "Identifier",
109-
// "name": "second"
110-
// }
111-
// ]
112-
// }
113-
setFilteredPosts(
114-
posts.filter(([, value]) => value !== "")
115-
);
116-
}, [posts]);
117-
}
118-
`,
119-
},
12094
{
12195
// https://github.com/NickvanDyke/eslint-plugin-react-you-might-not-need-an-effect/issues/16
12296
name: "External IIFE",
@@ -554,5 +528,38 @@ new MyRuleTester().run("/syntax", {
554528
},
555529
],
556530
},
531+
{
532+
name: "Destructured array skips element in arrow function params",
533+
code: js`
534+
function FilteredPosts() {
535+
const posts = useSomeAPI();
536+
const [filteredPosts, setFilteredPosts] = useState([]);
537+
538+
useEffect(() => {
539+
// Resulting AST node looks like:
540+
// {
541+
// "type": "ArrayPattern",
542+
// "elements": [
543+
// null, <-- Must handle this!
544+
// {
545+
// "type": "Identifier",
546+
// "name": "second"
547+
// }
548+
// ]
549+
// }
550+
setFilteredPosts(
551+
// TODO: Need to filter out parameter refs before comparing args and deps
552+
posts.filter(([, value]) => value !== "")
553+
);
554+
}, [posts]);
555+
}
556+
`,
557+
errors: [
558+
{
559+
messageId: messageIds.avoidDerivedState,
560+
data: { state: "filteredPosts" },
561+
},
562+
],
563+
},
557564
],
558565
});

0 commit comments

Comments
 (0)