@samitouri / QOS-React / commits / b1cccd1ed1

Warn about setState directly in dep-less useEffect (#15184)

Dan Abramov committed Mar 22, 2019 at 13:41 UTC b1cccd1ed14ff0d5db4fcefbf8b49807a48d7410

2 files changed +200 -37

packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js

+108

@@ -992,6 +992,18 @@ const tests = {

992 }

993 `,

994 },

995 + {

996 + code: `

997 + function Hello() {

998 + const [state, setState] = useState(0);

999 + useEffect(() => {

1000 + const handleResize = () => setState(window.innerWidth);

1001 + window.addEventListener('resize', handleResize);

1002 + return () => window.removeEventListener('resize', handleResize);

1003 + });

1004 + }

1005 + `,

1006 + },

1007 ],

1008 invalid: [

1009 {

@@ -4462,6 +4474,102 @@ const tests = {

4474 `Either exclude it or remove the dependency array.`,

4475 ],

4476 },

4477 + {

4478 + code: `

4479 + function Hello() {

4480 + const [state, setState] = useState(0);

4481 + useEffect(() => {

4482 + setState({});

4483 + });

4484 + }

4485 + `,

4486 + output: `

4487 + function Hello() {

4488 + const [state, setState] = useState(0);

4489 + useEffect(() => {

4490 + setState({});

4491 + }, []);

4492 + }

4493 + `,

4494 + errors: [

4495 + `React Hook useEffect contains a call to 'setState'. ` +

4496 + `Without a list of dependencies, this can lead to an infinite chain of updates. ` +

4497 + `To fix this, pass [] as a second argument to the useEffect Hook.`,

4498 + ],

4499 + },

4500 + {

4501 + code: `

4502 + function Hello() {

4503 + const [data, setData] = useState(0);

4504 + useEffect(() => {

4505 + fetchData.then(setData);

4506 + });

4507 + }

4508 + `,

4509 + output: `

4510 + function Hello() {

4511 + const [data, setData] = useState(0);

4512 + useEffect(() => {

4513 + fetchData.then(setData);

4514 + }, []);

4515 + }

4516 + `,

4517 + errors: [

4518 + `React Hook useEffect contains a call to 'setData'. ` +

4519 + `Without a list of dependencies, this can lead to an infinite chain of updates. ` +

4520 + `To fix this, pass [] as a second argument to the useEffect Hook.`,

4521 + ],

4522 + },

4523 + {

4524 + code: `

4525 + function Hello({ country }) {

4526 + const [data, setData] = useState(0);

4527 + useEffect(() => {

4528 + fetchData(country).then(setData);

4529 + });

4530 + }

4531 + `,

4532 + output: `

4533 + function Hello({ country }) {

4534 + const [data, setData] = useState(0);

4535 + useEffect(() => {

4536 + fetchData(country).then(setData);

4537 + }, [country]);

4538 + }

4539 + `,

4540 + errors: [

4541 + `React Hook useEffect contains a call to 'setData'. ` +

4542 + `Without a list of dependencies, this can lead to an infinite chain of updates. ` +

4543 + `To fix this, pass [country] as a second argument to the useEffect Hook.`,

4544 + ],

4545 + },

4546 + {

4547 + code: `

4548 + function Hello({ prop1, prop2 }) {

4549 + const [state, setState] = useState(0);

4550 + useEffect(() => {

4551 + if (prop1) {

4552 + setState(prop2);

4553 + }

4554 + });

4555 + }

4556 + `,

4557 + output: `

4558 + function Hello({ prop1, prop2 }) {

4559 + const [state, setState] = useState(0);

4560 + useEffect(() => {

4561 + if (prop1) {

4562 + setState(prop2);

4563 + }

4564 + }, [prop1, prop2]);

4565 + }

4566 + `,

4567 + errors: [

4568 + `React Hook useEffect contains a call to 'setState'. ` +

4569 + `Without a list of dependencies, this can lead to an infinite chain of updates. ` +

4570 + `To fix this, pass [prop1, prop2] as a second argument to the useEffect Hook.`,

4571 + ],

4572 + },

4573 {

4574 code: `

4575 function Thing() {

packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js

+92 -37

@@ -468,9 +468,101 @@ export default {

468 },

469 );

470

471 + // Warn about assigning to variables in the outer scope.

472 + // Those are usually bugs.

473 + let staleAssignments = new Set();

474 + function reportStaleAssignment(writeExpr, key) {

475 + if (staleAssignments.has(key)) {

476 + return;

477 + }

478 + staleAssignments.add(key);

479 + context.report({

480 + node: writeExpr,

481 + message:

482 + `Assignments to the '${key}' variable from inside React Hook ` +

483 + `${context.getSource(reactiveHook)} will be lost after each ` +

484 + `render. To preserve the value over time, store it in a useRef ` +

485 + `Hook and keep the mutable value in the '.current' property. ` +

486 + `Otherwise, you can move this variable directly inside ` +

487 + `${context.getSource(reactiveHook)}.`,

488 + });

489 + }

490 +

491 + // Remember which deps are optional and report bad usage first.

492 + const optionalDependencies = new Set();

493 + dependencies.forEach(({isStatic, references}, key) => {

494 + if (isStatic) {

495 + optionalDependencies.add(key);

496 + }

497 + references.forEach(reference => {

498 + if (reference.writeExpr) {

499 + reportStaleAssignment(reference.writeExpr, key);

500 + }

501 + });

502 + });

503 +

504 + if (staleAssignments.size > 0) {

505 + // The intent isn't clear so we'll wait until you fix those first.

506 + return;

507 + }

508 +

509 if (!declaredDependenciesNode) {

510 + // Check if there are any top-level setState() calls.

511 + // Those tend to lead to infinite loops.

512 + let setStateInsideEffectWithoutDeps = null;

513 + dependencies.forEach(({isStatic, references}, key) => {

514 + if (setStateInsideEffectWithoutDeps) {

515 + return;

516 + }

517 + references.forEach(reference => {

518 + if (setStateInsideEffectWithoutDeps) {

519 + return;

520 + }

521 +

522 + const id = reference.identifier;

523 + const isSetState = setStateCallSites.has(id);

524 + if (!isSetState) {

525 + return;

526 + }

527 +

528 + let fnScope = reference.from;

529 + while (fnScope.type !== 'function') {

530 + fnScope = fnScope.upper;

531 + }

532 + const isDirectlyInsideEffect = fnScope.block === node;

533 + if (isDirectlyInsideEffect) {

534 + // TODO: we could potentially ignore early returns.

535 + setStateInsideEffectWithoutDeps = key;

536 + }

537 + });

538 + });

539 + if (setStateInsideEffectWithoutDeps) {

540 + let {suggestedDependencies} = collectRecommendations({

541 + dependencies,

542 + declaredDependencies: [],

543 + optionalDependencies,

544 + externalDependencies: new Set(),

545 + isEffect: true,

546 + });

547 + context.report({

548 + node: node.parent.callee,

549 + message:

550 + `React Hook ${reactiveHookName} contains a call to '${setStateInsideEffectWithoutDeps}'. ` +

551 + `Without a list of dependencies, this can lead to an infinite chain of updates. ` +

552 + `To fix this, pass [` +

553 + suggestedDependencies.join(', ') +

554 + `] as a second argument to the ${reactiveHookName} Hook.`,

555 + fix(fixer) {

556 + return fixer.insertTextAfter(

557 + node,

558 + `, [${suggestedDependencies.join(', ')}]`,

559 + );

560 + },

561 + });

562 + }

563 return;

564 }

565 +

566 const declaredDependencies = [];

567 const externalDependencies = new Set();

568 if (declaredDependenciesNode.type !== 'ArrayExpression') {

@@ -569,43 +661,6 @@ export default {

661 });

662 }

663

572 - // Warn about assigning to variables in the outer scope.

573 - // Those are usually bugs.

574 - let staleAssignments = new Set();

575 - function reportStaleAssignment(writeExpr, key) {

576 - if (staleAssignments.has(key)) {

577 - return;

578 - }

579 - staleAssignments.add(key);

580 - context.report({

581 - node: writeExpr,

582 - message:

583 - `Assignments to the '${key}' variable from inside React Hook ` +

584 - `${context.getSource(reactiveHook)} will be lost after each ` +

585 - `render. To preserve the value over time, store it in a useRef ` +

586 - `Hook and keep the mutable value in the '.current' property. ` +

587 - `Otherwise, you can move this variable directly inside ` +

588 - `${context.getSource(reactiveHook)}.`,

589 - });

590 - }

591 -

592 - // Remember which deps are optional and report bad usage first.

593 - const optionalDependencies = new Set();

594 - dependencies.forEach(({isStatic, references}, key) => {

595 - if (isStatic) {

596 - optionalDependencies.add(key);

597 - }

598 - references.forEach(reference => {

599 - if (reference.writeExpr) {

600 - reportStaleAssignment(reference.writeExpr, key);

601 - }

602 - });

603 - });

604 - if (staleAssignments.size > 0) {

605 - // The intent isn't clear so we'll wait until you fix those first.

606 - return;

607 - }

608 -

664 let {

665 suggestedDependencies,

666 unnecessaryDependencies,