@samitouri / QOS-React / commits / e1e45fb367

[ESLint] Suggest to destructure props when they are only used as members (#14993)

* Suggest to destructure props when they are only used as members * Add more tests * Fix a bug

Dan Abramov committed Mar 1, 2019 at 19:48 UTC e1e45fb3673b7d23b4f89ab9fa9644fe9539eb9c
2 files changed +223 -1
packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
+183 -1
@@ -2224,7 +2224,189 @@ const tests = {
2224 }
2225 `,
2226 errors: [
2227 - // TODO: make this message clearer since it's not obvious why.
2227 + "React Hook useEffect has a missing dependency: 'props'. " +
2228 + 'Either include it or remove the dependency array. ' +
2229 + 'Alternatively, destructure the necessary props outside the callback.',
2230 + ],
2231 + },
2232 + {
2233 + code: `
2234 + function MyComponent(props) {
2235 + useEffect(() => {
2236 + function play() {
2237 + props.onPlay();
2238 + }
2239 + function pause() {
2240 + props.onPause();
2241 + }
2242 + }, []);
2243 + }
2244 + `,
2245 + output: `
2246 + function MyComponent(props) {
2247 + useEffect(() => {
2248 + function play() {
2249 + props.onPlay();
2250 + }
2251 + function pause() {
2252 + props.onPause();
2253 + }
2254 + }, [props]);
2255 + }
2256 + `,
2257 + errors: [
2258 + "React Hook useEffect has a missing dependency: 'props'. " +
2259 + 'Either include it or remove the dependency array. ' +
2260 + 'Alternatively, destructure the necessary props outside the callback.',
2261 + ],
2262 + },
2263 + {
2264 + code: `
2265 + function MyComponent(props) {
2266 + useEffect(() => {
2267 + if (props.foo.onChange) {
2268 + props.foo.onChange();
2269 + }
2270 + }, []);
2271 + }
2272 + `,
2273 + output: `
2274 + function MyComponent(props) {
2275 + useEffect(() => {
2276 + if (props.foo.onChange) {
2277 + props.foo.onChange();
2278 + }
2279 + }, [props.foo]);
2280 + }
2281 + `,
2282 + errors: [
2283 + "React Hook useEffect has a missing dependency: 'props.foo'. " +
2284 + 'Either include it or remove the dependency array.',
2285 + ],
2286 + },
2287 + {
2288 + code: `
2289 + function MyComponent(props) {
2290 + useEffect(() => {
2291 + props.onChange();
2292 + if (props.foo.onChange) {
2293 + props.foo.onChange();
2294 + }
2295 + }, []);
2296 + }
2297 + `,
2298 + output: `
2299 + function MyComponent(props) {
2300 + useEffect(() => {
2301 + props.onChange();
2302 + if (props.foo.onChange) {
2303 + props.foo.onChange();
2304 + }
2305 + }, [props]);
2306 + }
2307 + `,
2308 + errors: [
2309 + "React Hook useEffect has a missing dependency: 'props'. " +
2310 + 'Either include it or remove the dependency array. ' +
2311 + 'Alternatively, destructure the necessary props outside the callback.',
2312 + ],
2313 + },
2314 + {
2315 + code: `
2316 + function MyComponent(props) {
2317 + const [skillsCount] = useState();
2318 + useEffect(() => {
2319 + if (skillsCount === 0 && !props.isEditMode) {
2320 + props.toggleEditMode();
2321 + }
2322 + }, [skillsCount, props.isEditMode, props.toggleEditMode]);
2323 + }
2324 + `,
2325 + output: `
2326 + function MyComponent(props) {
2327 + const [skillsCount] = useState();
2328 + useEffect(() => {
2329 + if (skillsCount === 0 && !props.isEditMode) {
2330 + props.toggleEditMode();
2331 + }
2332 + }, [skillsCount, props.isEditMode, props.toggleEditMode, props]);
2333 + }
2334 + `,
2335 + errors: [
2336 + "React Hook useEffect has a missing dependency: 'props'. " +
2337 + 'Either include it or remove the dependency array. ' +
2338 + 'Alternatively, destructure the necessary props outside the callback.',
2339 + ],
2340 + },
2341 + {
2342 + code: `
2343 + function MyComponent(props) {
2344 + const [skillsCount] = useState();
2345 + useEffect(() => {
2346 + if (skillsCount === 0 && !props.isEditMode) {
2347 + props.toggleEditMode();
2348 + }
2349 + }, []);
2350 + }
2351 + `,
2352 + output: `
2353 + function MyComponent(props) {
2354 + const [skillsCount] = useState();
2355 + useEffect(() => {
2356 + if (skillsCount === 0 && !props.isEditMode) {
2357 + props.toggleEditMode();
2358 + }
2359 + }, [props, skillsCount]);
2360 + }
2361 + `,
2362 + errors: [
2363 + "React Hook useEffect has missing dependencies: 'props' and 'skillsCount'. " +
2364 + 'Either include them or remove the dependency array. ' +
2365 + 'Alternatively, destructure the necessary props outside the callback.',
2366 + ],
2367 + },
2368 + {
2369 + code: `
2370 + function MyComponent(props) {
2371 + useEffect(() => {
2372 + externalCall(props);
2373 + props.onChange();
2374 + }, []);
2375 + }
2376 + `,
2377 + output: `
2378 + function MyComponent(props) {
2379 + useEffect(() => {
2380 + externalCall(props);
2381 + props.onChange();
2382 + }, [props]);
2383 + }
2384 + `,
2385 + // Don't suggest to destructure props here since you can't.
2386 + errors: [
2387 + "React Hook useEffect has a missing dependency: 'props'. " +
2388 + 'Either include it or remove the dependency array.',
2389 + ],
2390 + },
2391 + {
2392 + code: `
2393 + function MyComponent(props) {
2394 + useEffect(() => {
2395 + props.onChange();
2396 + externalCall(props);
2397 + }, []);
2398 + }
2399 + `,
2400 + output: `
2401 + function MyComponent(props) {
2402 + useEffect(() => {
2403 + props.onChange();
2404 + externalCall(props);
2405 + }, [props]);
2406 + }
2407 + `,
2408 + // Don't suggest to destructure props here since you can't.
2409 + errors: [
2410 "React Hook useEffect has a missing dependency: 'props'. " +
2411 'Either include it or remove the dependency array.',
2412 ],
packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
+40
@@ -449,6 +449,46 @@ export default {
449 }
450 }
451
452 + // `props.foo()` marks `props` as a dependency because it has
453 + // a `this` value. This warning can be confusing.
454 + // So if we're going to show it, append a clarification.
455 + if (!extraWarning && missingDependencies.has('props')) {
456 + let propDep = dependencies.get('props');
457 + if (propDep == null) {
458 + return;
459 + }
460 + const refs = propDep.reference.resolved.references;
461 + if (!Array.isArray(refs)) {
462 + return;
463 + }
464 + let isPropsOnlyUsedInMembers = true;
465 + for (let i = 0; i < refs.length; i++) {
466 + const ref = refs[i];
467 + const id = fastFindReferenceWithParent(
468 + componentScope.block,
469 + ref.identifier,
470 + );
471 + if (!id) {
472 + isPropsOnlyUsedInMembers = false;
473 + break;
474 + }
475 + const parent = id.parent;
476 + if (parent == null) {
477 + isPropsOnlyUsedInMembers = false;
478 + break;
479 + }
480 + if (parent.type !== 'MemberExpression') {
481 + isPropsOnlyUsedInMembers = false;
482 + break;
483 + }
484 + }
485 + if (isPropsOnlyUsedInMembers) {
486 + extraWarning =
487 + ' Alternatively, destructure the necessary props ' +
488 + 'outside the callback.';
489 + }
490 + }
491 +
492 context.report({
493 node: declaredDependenciesNode,
494 message: