@samitouri / QOS-React-2 / commits / da54641a10

[ESLint] Check deps when callback body is outside the Hook call, too (#18435)

* Refactor: visit CallExpression Instead of visiting the functions and looking up to see if they're in a Hook call, visit Hook calls and look down to see if there's a callback inside. I will need this refactor so I can visit functions declared outside the call. * Check deps when callback body is outside the Hook call * Handle the unknown case

Dan Abramov committed Mar 31, 2020 at 02:09 UTC da54641a1022edc789c0290e6aa6421d6b9543c0
2 files changed +462 -28
packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
+338
@@ -259,6 +259,104 @@ const tests = {
259 }
260 `,
261 },
262 + {
263 + code: normalizeIndent`
264 + function MyComponent() {
265 + const myEffect = () => {
266 + // Doesn't use anything
267 + };
268 + useEffect(myEffect, []);
269 + }
270 + `,
271 + },
272 + {
273 + code: normalizeIndent`
274 + const local = {};
275 + function MyComponent() {
276 + const myEffect = () => {
277 + console.log(local);
278 + };
279 + useEffect(myEffect, []);
280 + }
281 + `,
282 + },
283 + {
284 + code: normalizeIndent`
285 + const local = {};
286 + function MyComponent() {
287 + function myEffect() {
288 + console.log(local);
289 + }
290 + useEffect(myEffect, []);
291 + }
292 + `,
293 + },
294 + {
295 + code: normalizeIndent`
296 + function MyComponent() {
297 + const local = {};
298 + function myEffect() {
299 + console.log(local);
300 + }
301 + useEffect(myEffect, [local]);
302 + }
303 + `,
304 + },
305 + {
306 + code: normalizeIndent`
307 + function MyComponent() {
308 + function myEffect() {
309 + console.log(global);
310 + }
311 + useEffect(myEffect, []);
312 + }
313 + `,
314 + },
315 + {
316 + code: normalizeIndent`
317 + const local = {};
318 + function MyComponent() {
319 + const myEffect = () => {
320 + otherThing()
321 + }
322 + const otherThing = () => {
323 + console.log(local);
324 + }
325 + useEffect(myEffect, []);
326 + }
327 + `,
328 + },
329 + {
330 + // Valid because even though we don't inspect the function itself,
331 + // at least it's passed as a dependency.
332 + code: normalizeIndent`
333 + function MyComponent({delay}) {
334 + const local = {};
335 + const myEffect = debounce(() => {
336 + console.log(local);
337 + }, delay);
338 + useEffect(myEffect, [myEffect]);
339 + }
340 + `,
341 + },
342 + {
343 + code: normalizeIndent`
344 + let local = {};
345 + function myEffect() {
346 + console.log(local);
347 + }
348 + function MyComponent() {
349 + useEffect(myEffect, []);
350 + }
351 + `,
352 + },
353 + {
354 + code: normalizeIndent`
355 + function MyComponent({myEffect}) {
356 + useEffect(myEffect, [myEffect]);
357 + }
358 + `,
359 + },
360 {
361 code: normalizeIndent`
362 function MyComponent(props) {
@@ -5998,6 +6096,246 @@ const tests = {
6096 },
6097 ],
6098 },
6099 + {
6100 + code: normalizeIndent`
6101 + function MyComponent() {
6102 + const local = {};
6103 + function myEffect() {
6104 + console.log(local);
6105 + }
6106 + useEffect(myEffect, []);
6107 + }
6108 + `,
6109 + errors: [
6110 + {
6111 + message:
6112 + "React Hook useEffect has a missing dependency: 'local'. " +
6113 + 'Either include it or remove the dependency array.',
6114 + suggestions: [
6115 + {
6116 + desc: 'Update the dependencies array to be: [local]',
6117 + output: normalizeIndent`
6118 + function MyComponent() {
6119 + const local = {};
6120 + function myEffect() {
6121 + console.log(local);
6122 + }
6123 + useEffect(myEffect, [local]);
6124 + }
6125 + `,
6126 + },
6127 + ],
6128 + },
6129 + ],
6130 + },
6131 + {
6132 + code: normalizeIndent`
6133 + function MyComponent() {
6134 + const local = {};
6135 + const myEffect = () => {
6136 + console.log(local);
6137 + };
6138 + useEffect(myEffect, []);
6139 + }
6140 + `,
6141 + errors: [
6142 + {
6143 + message:
6144 + "React Hook useEffect has a missing dependency: 'local'. " +
6145 + 'Either include it or remove the dependency array.',
6146 + suggestions: [
6147 + {
6148 + desc: 'Update the dependencies array to be: [local]',
6149 + output: normalizeIndent`
6150 + function MyComponent() {
6151 + const local = {};
6152 + const myEffect = () => {
6153 + console.log(local);
6154 + };
6155 + useEffect(myEffect, [local]);
6156 + }
6157 + `,
6158 + },
6159 + ],
6160 + },
6161 + ],
6162 + },
6163 + {
6164 + code: normalizeIndent`
6165 + function MyComponent() {
6166 + const local = {};
6167 + const myEffect = function() {
6168 + console.log(local);
6169 + };
6170 + useEffect(myEffect, []);
6171 + }
6172 + `,
6173 + errors: [
6174 + {
6175 + message:
6176 + "React Hook useEffect has a missing dependency: 'local'. " +
6177 + 'Either include it or remove the dependency array.',
6178 + suggestions: [
6179 + {
6180 + desc: 'Update the dependencies array to be: [local]',
6181 + output: normalizeIndent`
6182 + function MyComponent() {
6183 + const local = {};
6184 + const myEffect = function() {
6185 + console.log(local);
6186 + };
6187 + useEffect(myEffect, [local]);
6188 + }
6189 + `,
6190 + },
6191 + ],
6192 + },
6193 + ],
6194 + },
6195 + {
6196 + code: normalizeIndent`
6197 + function MyComponent() {
6198 + const local = {};
6199 + const myEffect = () => {
6200 + otherThing();
6201 + };
6202 + const otherThing = () => {
6203 + console.log(local);
6204 + };
6205 + useEffect(myEffect, []);
6206 + }
6207 + `,
6208 + errors: [
6209 + {
6210 + message:
6211 + "React Hook useEffect has a missing dependency: 'otherThing'. " +
6212 + 'Either include it or remove the dependency array.',
6213 + suggestions: [
6214 + {
6215 + desc: 'Update the dependencies array to be: [otherThing]',
6216 + output: normalizeIndent`
6217 + function MyComponent() {
6218 + const local = {};
6219 + const myEffect = () => {
6220 + otherThing();
6221 + };
6222 + const otherThing = () => {
6223 + console.log(local);
6224 + };
6225 + useEffect(myEffect, [otherThing]);
6226 + }
6227 + `,
6228 + },
6229 + ],
6230 + },
6231 + ],
6232 + },
6233 + {
6234 + code: normalizeIndent`
6235 + function MyComponent() {
6236 + const local = {};
6237 + const myEffect = debounce(() => {
6238 + console.log(local);
6239 + }, delay);
6240 + useEffect(myEffect, []);
6241 + }
6242 + `,
6243 + errors: [
6244 + {
6245 + message:
6246 + "React Hook useEffect has a missing dependency: 'myEffect'. " +
6247 + 'Either include it or remove the dependency array.',
6248 + suggestions: [
6249 + {
6250 + desc: 'Update the dependencies array to be: [myEffect]',
6251 + output: normalizeIndent`
6252 + function MyComponent() {
6253 + const local = {};
6254 + const myEffect = debounce(() => {
6255 + console.log(local);
6256 + }, delay);
6257 + useEffect(myEffect, [myEffect]);
6258 + }
6259 + `,
6260 + },
6261 + ],
6262 + },
6263 + ],
6264 + },
6265 + {
6266 + code: normalizeIndent`
6267 + function MyComponent() {
6268 + const local = {};
6269 + const myEffect = debounce(() => {
6270 + console.log(local);
6271 + }, delay);
6272 + useEffect(myEffect, [local]);
6273 + }
6274 + `,
6275 + errors: [
6276 + {
6277 + message:
6278 + "React Hook useEffect has a missing dependency: 'myEffect'. " +
6279 + 'Either include it or remove the dependency array.',
6280 + suggestions: [
6281 + {
6282 + desc: 'Update the dependencies array to be: [myEffect]',
6283 + output: normalizeIndent`
6284 + function MyComponent() {
6285 + const local = {};
6286 + const myEffect = debounce(() => {
6287 + console.log(local);
6288 + }, delay);
6289 + useEffect(myEffect, [myEffect]);
6290 + }
6291 + `,
6292 + },
6293 + ],
6294 + },
6295 + ],
6296 + },
6297 + {
6298 + code: normalizeIndent`
6299 + function MyComponent({myEffect}) {
6300 + useEffect(myEffect, []);
6301 + }
6302 + `,
6303 + errors: [
6304 + {
6305 + message:
6306 + "React Hook useEffect has a missing dependency: 'myEffect'. " +
6307 + 'Either include it or remove the dependency array.',
6308 + suggestions: [
6309 + {
6310 + desc: 'Update the dependencies array to be: [myEffect]',
6311 + output: normalizeIndent`
6312 + function MyComponent({myEffect}) {
6313 + useEffect(myEffect, [myEffect]);
6314 + }
6315 + `,
6316 + },
6317 + ],
6318 + },
6319 + ],
6320 + },
6321 + {
6322 + code: normalizeIndent`
6323 + function MyComponent() {
6324 + const local = {};
6325 + useEffect(debounce(() => {
6326 + console.log(local);
6327 + }, delay), []);
6328 + }
6329 + `,
6330 + errors: [
6331 + {
6332 + message:
6333 + 'React Hook useEffect received a function whose dependencies ' +
6334 + 'are unknown. Pass an inline function instead.',
6335 + suggestions: [],
6336 + },
6337 + ],
6338 + },
6339 ],
6340 };
6341
packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
+124 -28
@@ -33,6 +33,8 @@ export default {
33 : undefined;
34 const options = {additionalHooks};
35
36 + const scopeManager = context.getSourceCode().scopeManager;
37 +
38 // Should be shared between visitors.
39 let setStateCallSites = new WeakMap();
40 let stateVariables = new WeakSet();
@@ -52,41 +54,135 @@ export default {
54 }
55
56 return {
55 - FunctionExpression: visitFunctionExpression,
56 - ArrowFunctionExpression: visitFunctionExpression,
57 + CallExpression: visitCallExpression,
58 };
59
59 - /**
60 - * Visitor for both function expressions and arrow function expressions.
61 - */
62 - function visitFunctionExpression(node) {
63 - // We only want to lint nodes which are reactive hook callbacks.
64 - if (
65 - (node.type !== 'FunctionExpression' &&
66 - node.type !== 'ArrowFunctionExpression') ||
67 - node.parent.type !== 'CallExpression'
68 - ) {
60 + function visitCallExpression(node) {
61 + const callbackIndex = getReactiveHookCallbackIndex(node.callee, options);
62 + if (callbackIndex === -1) {
63 + // Not a React Hook call that needs deps.
64 return;
65 }
71 -
72 - const callbackIndex = getReactiveHookCallbackIndex(
73 - node.parent.callee,
74 - options,
75 - );
76 - if (node.parent.arguments[callbackIndex] !== node) {
77 - return;
66 + const callback = node.arguments[callbackIndex];
67 + const reactiveHook = node.callee;
68 + const reactiveHookName = getNodeWithoutReactNamespace(reactiveHook).name;
69 + const declaredDependenciesNode = node.arguments[callbackIndex + 1];
70 +
71 + switch (callback.type) {
72 + case 'FunctionExpression':
73 + case 'ArrowFunctionExpression':
74 + visitFunctionWithDependencies(
75 + callback,
76 + declaredDependenciesNode,
77 + reactiveHook,
78 + );
79 + return; // Handled
80 + case 'Identifier':
81 + // The function passed as a callback is not written inline.
82 + // But perhaps it's in the dependencies array?
83 + if (
84 + declaredDependenciesNode &&
85 + declaredDependenciesNode.elements &&
86 + declaredDependenciesNode.elements.some(
87 + el => el.type === 'Identifier' && el.name === callback.name,
88 + )
89 + ) {
90 + // If it's already in the list of deps, we don't care because
91 + // this is valid regardless.
92 + return; // Handled
93 + }
94 + // We'll do our best effort to find it, complain otherwise.
95 + const variable = context.getScope().set.get(callback.name);
96 + if (variable == null || variable.defs == null) {
97 + // If it's not in scope, we don't care.
98 + return; // Handled
99 + }
100 + // The function passed as a callback is not written inline.
101 + // But it's defined somewhere in the render scope.
102 + // We'll do our best effort to find and check it, complain otherwise.
103 + const def = variable.defs[0];
104 + if (!def || !def.node) {
105 + break; // Unhandled
106 + }
107 + if (def.type !== 'Variable' && def.type !== 'FunctionName') {
108 + // Parameter or an unusual pattern. Bail out.
109 + break; // Unhandled
110 + }
111 + switch (def.node.type) {
112 + case 'FunctionDeclaration':
113 + // useEffect(() => { ... }, []);
114 + visitFunctionWithDependencies(
115 + def.node,
116 + declaredDependenciesNode,
117 + reactiveHook,
118 + );
119 + return; // Handled
120 + case 'VariableDeclarator':
121 + const init = def.node.init;
122 + if (!init) {
123 + break; // Unhandled
124 + }
125 + switch (init.type) {
126 + // const effectBody = () => {...};
127 + // useEffect(effectBody, []);
128 + case 'ArrowFunctionExpression':
129 + case 'FunctionExpression':
130 + // We can inspect this function as if it were inline.
131 + visitFunctionWithDependencies(
132 + init,
133 + declaredDependenciesNode,
134 + reactiveHook,
135 + );
136 + return; // Handled
137 + }
138 + break; // Unhandled
139 + }
140 + break; // Unhandled
141 + default:
142 + // useEffect(generateEffectBody(), []);
143 + context.report({
144 + node: reactiveHook,
145 + message:
146 + `React Hook ${reactiveHookName} received a function whose dependencies ` +
147 + `are unknown. Pass an inline function instead.`,
148 + });
149 + return; // Handled
150 }
151
80 - // Get the reactive hook node.
81 - const reactiveHook = node.parent.callee;
152 + // Something unusual. Fall back to suggesting to add the body itself as a dep.
153 + context.report({
154 + node: reactiveHook,
155 + message:
156 + `React Hook ${reactiveHookName} has a missing dependency: '${callback.name}'. ` +
157 + `Either include it or remove the dependency array.`,
158 + suggest: [
159 + {
160 + desc: `Update the dependencies array to be: [${callback.name}]`,
161 + fix(fixer) {
162 + return fixer.replaceText(
163 + declaredDependenciesNode,
164 + `[${callback.name}]`,
165 + );
166 + },
167 + },
168 + ],
169 + });
170 + }
171 +
172 + /**
173 + * Visitor for both function expressions and arrow function expressions.
174 + */
175 + function visitFunctionWithDependencies(
176 + node,
177 + declaredDependenciesNode,
178 + reactiveHook,
179 + ) {
180 const reactiveHookName = getNodeWithoutReactNamespace(reactiveHook).name;
181 const isEffect = /Effect($|[^a-z])/g.test(reactiveHookName);
182
85 - // Get the declared dependencies for this reactive hook. If there is no
183 + // Check the declared dependencies for this reactive hook. If there is no
184 // second argument then the reactive callback will re-run on every render.
185 // So no need to check for dependency inclusion.
88 - const depsIndex = callbackIndex + 1;
89 - const declaredDependenciesNode = node.parent.arguments[depsIndex];
186 if (!declaredDependenciesNode && !isEffect) {
187 // These are only used for optimization.
188 if (
@@ -95,7 +191,7 @@ export default {
191 ) {
192 // TODO: Can this have a suggestion?
193 context.report({
98 - node: node.parent.callee,
194 + node: reactiveHook,
195 message:
196 `React Hook ${reactiveHookName} does nothing when called with ` +
197 `only one argument. Did you forget to pass an array of ` +
@@ -124,7 +220,7 @@ export default {
220 }
221
222 // Get the current scope.
127 - const scope = context.getScope();
223 + const scope = scopeManager.acquire(node);
224
225 // Find all our "pure scopes". On every re-render of a component these
226 // pure scopes may have changes to the variables declared within. So all
@@ -550,7 +646,7 @@ export default {
646 isEffect: true,
647 });
648 context.report({
553 - node: node.parent.callee,
649 + node: reactiveHook,
650 message:
651 `React Hook ${reactiveHookName} contains a call to '${setStateInsideEffectWithoutDeps}'. ` +
652 `Without a list of dependencies, this can lead to an infinite chain of updates. ` +
@@ -1341,7 +1437,7 @@ function getNodeWithoutReactNamespace(node, options) {
1437 function getReactiveHookCallbackIndex(calleeNode, options) {
1438 let node = getNodeWithoutReactNamespace(calleeNode);
1439 if (node.type !== 'Identifier') {
1344 - return null;
1440 + return -1;
1441 }
1442 switch (node.name) {
1443 case 'useEffect':