From 7d160616bb71a20223f2d82e290142a2148a9a8d Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Thu, 5 May 2022 11:18:17 -0700 Subject: [PATCH 1/2] Add eslint rule to error on reads or writes to refs in render --- .../__tests__/ESLintPureRender-test.js | 202 ++++++++++++++++++ .../src/PureRender.js | 180 ++++++++++++++++ .../eslint-plugin-react-hooks/src/index.js | 2 + 3 files changed, 384 insertions(+) create mode 100644 packages/eslint-plugin-react-hooks/__tests__/ESLintPureRender-test.js create mode 100644 packages/eslint-plugin-react-hooks/src/PureRender.js diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintPureRender-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintPureRender-test.js new file mode 100644 index 000000000000..7518e36478a1 --- /dev/null +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintPureRender-test.js @@ -0,0 +1,202 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +'use strict'; + +const ESLintTester = require('eslint').RuleTester; +const ReactHooksESLintPlugin = require('eslint-plugin-react-hooks'); +const PureRenderESLintRule = ReactHooksESLintPlugin.rules['pure-render']; + +ESLintTester.setDefaultConfig({ + parser: require.resolve('babel-eslint'), + parserOptions: { + ecmaVersion: 6, + sourceType: 'module', + }, +}); + +const writeError = { + message: + 'Writing to refs during rendering is not allowed. Move this into a useEffect or useLayoutEffect. See https://beta.reactjs.org/apis/useref', +}; +const readError = { + message: + 'Reading from refs during rendering is not allowed. See https://beta.reactjs.org/apis/useref', +}; + +const tests = { + valid: [ + { + code: ` + function MyComponent() { + let ref = useRef(false); + useLayoutEffect(() => { + ref.current = true; + }); + } + `, + }, + { + code: ` + let MyComponent = () => { + let ref = useRef(false); + useLayoutEffect(() => { + ref.current = true; + }); + }; + `, + }, + { + code: ` + let MyComponent = function () { + let ref = useRef(false); + useLayoutEffect(() => { + ref.current = true; + }); + }; + `, + }, + { + code: ` + function MyComponent() { + let ref = useRef(false); + let onChange = () => { + ref.current = true; + }; + } + `, + }, + { + code: ` + function MyComponent() { + let ref = useRef(null); + if (ref.current == null) { + ref.current = somethingExpensive(); + } + } + `, + }, + { + code: ` + function MyComponent() { + let ref = useRef(false); + if (ref.current === false) { + ref.current = somethingExpensive(); + } + } + `, + }, + { + code: ` + function MyComponent() { + let ref = useRef(undefined); + if (ref.current === undefined) { + ref.current = somethingExpensive(); + } + } + `, + }, + { + code: ` + function MyComponent() { + let ref = useRef(undefined); + if (ref.current == null) { + ref.current = somethingExpensive(); + } + } + `, + }, + { + code: ` + function MyComponent() { + let ref = useRef(); + if (ref.current == null) { + ref.current = somethingExpensive(); + } + } + `, + }, + { + code: ` + function MyComponent() { + let ref = useRef(); + if (ref.current == null) { + ref.current = somethingExpensive(); + } + } + `, + }, + ], + invalid: [ + { + code: ` + function MyComponent() { + let ref = useRef(false); + ref.current = true; + } + `, + errors: [writeError], + }, + { + code: ` + function MyComponent() { + let ref = useRef(false); + return

{ref.current}

; + } + `, + errors: [readError], + }, + { + code: ` + function MyComponent() { + let ref = useRef(false); + if (ref.current === 'test') { + ref.current = somethingExpensive(); + } + } + `, + errors: [readError, writeError], + }, + { + code: ` + function MyComponent() { + let ref = useRef(null); + if (ref.current === undefined) { + ref.current = somethingExpensive(); + } + } + `, + errors: [readError, writeError], + }, + { + code: ` + function MyComponent() { + let ref = useRef(undefined); + if (ref.current === null) { + ref.current = somethingExpensive(); + } + } + `, + errors: [readError, writeError], + }, + { + code: ` + function MyComponent() { + let ref = useRef(null); + let someOtherRef = useRef(null); + if (someOtherRef.current === null) { + ref.current = somethingExpensive(); + } + } + `, + errors: [writeError], + }, + ], +}; + +const eslintTester = new ESLintTester(); +eslintTester.run('react-hooks', PureRenderESLintRule, tests); diff --git a/packages/eslint-plugin-react-hooks/src/PureRender.js b/packages/eslint-plugin-react-hooks/src/PureRender.js new file mode 100644 index 000000000000..9d877cabe8a8 --- /dev/null +++ b/packages/eslint-plugin-react-hooks/src/PureRender.js @@ -0,0 +1,180 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +'use strict'; + +export default { + meta: { + type: 'problem', + docs: { + description: 'enforces component render purity', + recommended: true, + url: 'https://beta.reactjs.org/learn/keeping-components-pure', + }, + }, + create(context) { + return { + MemberExpression(member) { + // Look for member expressions that look like refs (i.e. `ref.current`). + if ( + member.object.type !== 'Identifier' || + member.computed || + member.property.type !== 'Identifier' || + member.property.name !== 'current' + ) { + return; + } + + // Find the parent function of this node, as well as any if statement matching against the ref value + // (i.e. lazy init pattern shown in React docs). + let node = member; + let fn; + let conditional; + while (node) { + if ( + node.type === 'FunctionDeclaration' || + node.type === 'FunctionExpression' || + node.type === 'ArrowFunctionExpression' + ) { + fn = node; + break; + } + + if ( + node.type === 'IfStatement' && + node.test.type === 'BinaryExpression' && + (node.test.operator === '==' || node.test.operator === '===') && + isMemberExpressionEqual(node.test.left, member) + ) { + conditional = node.test; + } + + node = node.parent; + } + + if (!fn) { + return; + } + + // Find the variable definition for the object. + const variable = getVariable(context.getScope(), member.object.name); + if (!variable) { + return; + } + + // Find the initialization of the variable and see if it's a call to useRef. + const refDefinition = variable.defs.find(def => { + const init = def.node.init; + if (!init) { + return false; + } + + return ( + init.type === 'CallExpression' && + init.callee.type === 'Identifier' && + init.callee.name === 'useRef' && + parentFunction(def.node) === fn + ); + }); + + if (refDefinition) { + // If within an if statement, check if comparing with the initial value passed to useRef. + // This indicates the lazy init pattern, which is allowed. + if (conditional) { + const init = refDefinition.node.init.arguments[0] || { + type: 'Identifier', + name: 'undefined', + }; + if (isLiteralEqual(conditional.operator, init, conditional.right)) { + return; + } + } + + // Otherwise, report an error for either writing or reading to this ref based on parent expression. + context.report({ + node: member, + message: + member.parent.type === 'AssignmentExpression' && + member.parent.left === member + ? `Writing to refs during rendering is not allowed. Move this into a useEffect or useLayoutEffect. See https://beta.reactjs.org/apis/useref` + : 'Reading from refs during rendering is not allowed. See https://beta.reactjs.org/apis/useref', + }); + } + }, + }; + }, +}; + +function getVariable(scope, name) { + while (scope) { + const variable = scope.set.get(name); + if (variable) { + return variable; + } + + scope = scope.upper; + } +} + +function parentFunction(node) { + while (node) { + if ( + node.type === 'FunctionDeclaration' || + node.type === 'FunctionExpression' || + node.type === 'ArrowFunctionExpression' + ) { + return node; + } + + node = node.parent; + } +} + +function isMemberExpressionEqual(a, b) { + if (a === b) { + return true; + } + + return ( + a.type === 'MemberExpression' && + b.type === 'MemberExpression' && + a.object.type === 'Identifier' && + b.object.type === 'Identifier' && + a.object.name === b.object.name && + a.property.type === 'Identifier' && + b.property.type === 'Identifier' && + a.property.name === b.property.name + ); +} + +function isLiteralEqual(operator, a, b) { + let aValue, bValue; + if (a.type === 'Identifier' && a.name === 'undefined') { + aValue = undefined; + } else if (a.type === 'Literal') { + aValue = a.value; + } else { + return; + } + + if (b.type === 'Identifier' && b.name === 'undefined') { + bValue = undefined; + } else if (b.type === 'Literal') { + bValue = b.value; + } else { + return; + } + + if (operator === '===') { + return aValue === bValue; + } else if (operator === '==') { + // eslint-disable-next-line + return aValue == bValue; + } + + return false; +} diff --git a/packages/eslint-plugin-react-hooks/src/index.js b/packages/eslint-plugin-react-hooks/src/index.js index 88e1ae71a2a7..3ef72b94579b 100644 --- a/packages/eslint-plugin-react-hooks/src/index.js +++ b/packages/eslint-plugin-react-hooks/src/index.js @@ -9,6 +9,7 @@ import RulesOfHooks from './RulesOfHooks'; import ExhaustiveDeps from './ExhaustiveDeps'; +import PureRender from './PureRender'; export const configs = { recommended: { @@ -23,4 +24,5 @@ export const configs = { export const rules = { 'rules-of-hooks': RulesOfHooks, 'exhaustive-deps': ExhaustiveDeps, + 'pure-render': PureRender, }; From 18d2eea2dc23eb8c98c205b6e813f66b3d6b9d70 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Thu, 5 May 2022 11:52:04 -0700 Subject: [PATCH 2/2] Support `React.useRef` too, and add more tests --- .../__tests__/ESLintPureRender-test.js | 37 +++++++++++++++++++ .../src/PureRender.js | 9 ++++- 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintPureRender-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintPureRender-test.js index 7518e36478a1..468029b0e63d 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintPureRender-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintPureRender-test.js @@ -130,6 +130,16 @@ const tests = { } `, }, + { + code: ` + function MyComponent() { + let ref = React.useRef(); + if (ref.current == null) { + ref.current = somethingExpensive(); + } + } + `, + }, ], invalid: [ { @@ -141,6 +151,33 @@ const tests = { `, errors: [writeError], }, + { + code: ` + let MyComponent = () => { + let ref = useRef(false); + ref.current = true; + }; + `, + errors: [writeError], + }, + { + code: ` + let MyComponent = function () { + let ref = useRef(false); + ref.current = true; + }; + `, + errors: [writeError], + }, + { + code: ` + function MyComponent() { + let ref = React.useRef(false); + ref.current = true; + } + `, + errors: [writeError], + }, { code: ` function MyComponent() { diff --git a/packages/eslint-plugin-react-hooks/src/PureRender.js b/packages/eslint-plugin-react-hooks/src/PureRender.js index 9d877cabe8a8..95cd6040ce2a 100644 --- a/packages/eslint-plugin-react-hooks/src/PureRender.js +++ b/packages/eslint-plugin-react-hooks/src/PureRender.js @@ -75,8 +75,13 @@ export default { return ( init.type === 'CallExpression' && - init.callee.type === 'Identifier' && - init.callee.name === 'useRef' && + ((init.callee.type === 'Identifier' && + init.callee.name === 'useRef') || + (init.callee.type === 'MemberExpression' && + init.callee.object.type === 'Identifier' && + init.callee.object.name === 'React' && + init.callee.property.type === 'Identifier' && + init.callee.property.name === 'useRef')) && parentFunction(def.node) === fn ); });