@samitouri / QOS-React / commits / a77bbf1a1c

[ESLint] Warn against assignments from inside Hooks (#14916)

* [ESLint] Warn against assignments from inside Hooks * Include variable name * Add a test for the legit case

Dan Abramov committed Feb 21, 2019 at 19:23 UTC a77bbf1a1c278a39dd7b1afb02f3d92402fcc8a0

2 files changed +161 -3

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

+135 -1

@@ -433,6 +433,19 @@ const tests = {

433 });

434 `,

435 },

436 + {

437 + // This is not ideal but warning would likely create

438 + // too many false positives. We do, however, prevent

439 + // direct assignments.

440 + code: `

441 + function MyComponent(props) {

442 + let obj = {};

443 + useEffect(() => {

444 + obj.foo = true;

445 + }, [obj]);

446 + }

447 + `,

448 + },

449 ],

450 invalid: [

451 {

@@ -891,7 +904,6 @@ const tests = {

904 ],

905 },

906 {

894 - // only: true,

907 code: `

908 function MyComponent(props) {

909 useEffect(() => {

@@ -1608,6 +1620,128 @@ const tests = {

1620 'Either include them or remove the dependency array.',

1621 ],

1622 },

1623 + {

1624 + code: `

1625 + function MyComponent(props) {

1626 + let value;

1627 + let value2;

1628 + let value3;

1629 + let asyncValue;

1630 + useEffect(() => {

1631 + value = 42;

1632 + value2 = 100;

1633 + value = 43;

1634 + console.log(value2);

1635 + console.log(value3);

1636 + setTimeout(() => {

1637 + asyncValue = 100;

1638 + });

1639 + }, []);

1640 + }

1641 + `,

1642 + // This is a separate warning unrelated to others.

1643 + // We could've made a separate rule for it but it's rare enough to name it.

1644 + // No autofix suggestion because the intent isn't clear.

1645 + output: `

1646 + function MyComponent(props) {

1647 + let value;

1648 + let value2;

1649 + let value3;

1650 + let asyncValue;

1651 + useEffect(() => {

1652 + value = 42;

1653 + value2 = 100;

1654 + value = 43;

1655 + console.log(value2);

1656 + console.log(value3);

1657 + setTimeout(() => {

1658 + asyncValue = 100;

1659 + });

1660 + }, []);

1661 + }

1662 + `,

1663 + errors: [

1664 + // value

1665 + `Assignments to the 'value' variable from inside a React useEffect Hook ` +

1666 + `will not persist between re-renders. ` +

1667 + `If it's only needed by this Hook, move the variable inside it. ` +

1668 + `Alternatively, declare a ref with the useRef Hook, ` +

1669 + `and keep the mutable value in its 'current' property.`,

1670 + // value2

1671 + `Assignments to the 'value2' variable from inside a React useEffect Hook ` +

1672 + `will not persist between re-renders. ` +

1673 + `If it's only needed by this Hook, move the variable inside it. ` +

1674 + `Alternatively, declare a ref with the useRef Hook, ` +

1675 + `and keep the mutable value in its 'current' property.`,

1676 + // asyncValue

1677 + `Assignments to the 'asyncValue' variable from inside a React useEffect Hook ` +

1678 + `will not persist between re-renders. ` +

1679 + `If it's only needed by this Hook, move the variable inside it. ` +

1680 + `Alternatively, declare a ref with the useRef Hook, ` +

1681 + `and keep the mutable value in its 'current' property.`,

1682 + ],

1683 + },

1684 + {

1685 + code: `

1686 + function MyComponent(props) {

1687 + let value;

1688 + let value2;

1689 + let value3;

1690 + let asyncValue;

1691 + useEffect(() => {

1692 + value = 42;

1693 + value2 = 100;

1694 + value = 43;

1695 + console.log(value2);

1696 + console.log(value3);

1697 + setTimeout(() => {

1698 + asyncValue = 100;

1699 + });

1700 + }, [value, value2, value3]);

1701 + }

1702 + `,

1703 + // This is a separate warning unrelated to others.

1704 + // We could've made a separate rule for it but it's rare enough to name it.

1705 + // No autofix suggestion because the intent isn't clear.

1706 + output: `

1707 + function MyComponent(props) {

1708 + let value;

1709 + let value2;

1710 + let value3;

1711 + let asyncValue;

1712 + useEffect(() => {

1713 + value = 42;

1714 + value2 = 100;

1715 + value = 43;

1716 + console.log(value2);

1717 + console.log(value3);

1718 + setTimeout(() => {

1719 + asyncValue = 100;

1720 + });

1721 + }, [value, value2, value3]);

1722 + }

1723 + `,

1724 + errors: [

1725 + // value

1726 + `Assignments to the 'value' variable from inside a React useEffect Hook ` +

1727 + `will not persist between re-renders. ` +

1728 + `If it's only needed by this Hook, move the variable inside it. ` +

1729 + `Alternatively, declare a ref with the useRef Hook, ` +

1730 + `and keep the mutable value in its 'current' property.`,

1731 + // value2

1732 + `Assignments to the 'value2' variable from inside a React useEffect Hook ` +

1733 + `will not persist between re-renders. ` +

1734 + `If it's only needed by this Hook, move the variable inside it. ` +

1735 + `Alternatively, declare a ref with the useRef Hook, ` +

1736 + `and keep the mutable value in its 'current' property.`,

1737 + // asyncValue

1738 + `Assignments to the 'asyncValue' variable from inside a React useEffect Hook ` +

1739 + `will not persist between re-renders. ` +

1740 + `If it's only needed by this Hook, move the variable inside it. ` +

1741 + `Alternatively, declare a ref with the useRef Hook, ` +

1742 + `and keep the mutable value in its 'current' property.`,

1743 + ],

1744 + },

1745 ],

1746 };

1747

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

+26 -2

@@ -186,7 +186,10 @@ export default {

186 // again in our dependencies array. Remember whether it's static.

187 if (!dependencies.has(dependency)) {

188 const isStatic = isDefinitelyStaticDependency(reference);

189 - dependencies.set(dependency, isStatic);

189 + dependencies.set(dependency, {

190 + isStatic,

191 + reference,

192 + });

193 }

194 }

195 for (const childScope of currentScope.childScopes) {

@@ -258,6 +261,7 @@ export default {

261 let unnecessaryDependencies = new Set();

262 let missingDependencies = new Set();

263 let actualDependencies = Array.from(dependencies.keys());

264 + let foundStaleAssignments = false;

265

266 function satisfies(actualDep, dep) {

267 return actualDep === dep || actualDep.startsWith(dep + '.');

@@ -280,7 +284,22 @@ export default {

284 });

285

286 // Then fill in the missing ones.

283 - dependencies.forEach((isStatic, key) => {

287 + dependencies.forEach(({isStatic, reference}, key) => {

288 + if (reference.writeExpr) {

289 + foundStaleAssignments = true;

290 + context.report({

291 + node: reference.writeExpr,

292 + message:

293 + `Assignments to the '${key}' variable from inside a React ${context.getSource(

294 + reactiveHook,

295 + )} Hook ` +

296 + `will not persist between re-renders. ` +

297 + `If it's only needed by this Hook, move the variable inside it. ` +

298 + `Alternatively, declare a ref with the useRef Hook, ` +

299 + `and keep the mutable value in its 'current' property.`,

300 + });

301 + }

302 +

303 if (

304 !suggestedDependencies.some(suggestedDep =>

305 satisfies(key, suggestedDep),

@@ -319,6 +338,11 @@ export default {

338 return;

339 }

340

341 + if (foundStaleAssignments) {

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

343 + return;

344 + }

345 +

346 function getWarningMessage(deps, singlePrefix, label, fixVerb) {

347 if (deps.size === 0) {

348 return null;