Add eslint-plugin-react-hooks/exhaustive-deps rule to check stale closure dependencies (#14636)
* Add ESLint rule for useEffect/useCallback/useMemo Hook dependencies * Fix ReactiveDependencies rule * fix lint errors * Support useLayoutEffect * Add some failing tests and comments * Gather dependencies in child scopes too * If we don't find foo.bar.baz in deps, try foo.bar, then foo * foo is enough for both foo.bar and foo.baz * Shorter rule name * Add fixable meta * Remove a bunch of code and start from scratch * [WIP] Only report errors from dependency array This results in nicer editing experience. Also has autofix. * Fix typo * [Temp] Skip all tests * Fix the first test * Revamp the test suite * Fix [foo] to include foo.bar * Don't suggest call expressions * Special case 'current' for refs * Don't complain about known static deps * Support useImperativeHandle * Better punctuation and formatting * More uniform message format * Treat React.useRef/useState/useReducer as static too * Add special message for ref.current * Add a TODO case * Alphabetize the autofix * Only alphabetize if it already was * Don't add static deps by default * Add an undefined variable case * Tweak wording * Rename to exhaustive-deps * Clean up / refactor a little bit
This commit is contained in:
Dan Abramov
GitHub
parent
1493abd7e0
commit
dab2fdbbbd
|
|
@ -9,6 +9,7 @@
|
|||
},
|
||||
"plugins": ["react-hooks"],
|
||||
"rules": {
|
||||
"react-hooks/rules-of-hooks": 2
|
||||
"react-hooks/rules-of-hooks": 2,
|
||||
"react-hooks/exhaustive-deps": 2
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -4,8 +4,26 @@
|
|||
// 2. "File > Add Folder to Workspace" this specific folder in VSCode with ESLint extension
|
||||
// 3. Changes to the rule source should get picked up without restarting ESLint server
|
||||
|
||||
function Foo() {
|
||||
if (condition) {
|
||||
useEffect(() => {});
|
||||
}
|
||||
function Comment({comment, commentSource}) {
|
||||
const currentUserID = comment.viewer.id;
|
||||
const environment = RelayEnvironment.forUser(currentUserID);
|
||||
const commentID = nullthrows(comment.id);
|
||||
useEffect(
|
||||
() => {
|
||||
const subscription = SubscriptionCounter.subscribeOnce(
|
||||
`StoreSubscription_${commentID}`,
|
||||
() =>
|
||||
StoreSubscription.subscribe(
|
||||
environment,
|
||||
{
|
||||
comment_id: commentID,
|
||||
},
|
||||
currentUserID,
|
||||
commentSource
|
||||
)
|
||||
);
|
||||
return () => subscription.dispose();
|
||||
},
|
||||
[commentID, commentSource, currentUserID, environment]
|
||||
);
|
||||
}
|
||||
|
|
|
|||
File diff suppressed because it is too large
Load Diff
|
|
@ -0,0 +1,567 @@
|
|||
/**
|
||||
* 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.
|
||||
*/
|
||||
|
||||
/* eslint-disable no-for-of-loops/no-for-of-loops */
|
||||
|
||||
'use strict';
|
||||
|
||||
// const [state, setState] = useState() / React.useState()
|
||||
// ^^^ true for this reference
|
||||
// const [state, dispatch] = useReducer() / React.useReducer()
|
||||
// ^^^ true for this reference
|
||||
// const ref = useRef()
|
||||
// ^^ true for this reference
|
||||
// False for everything else.
|
||||
function isDefinitelyStaticDependency(reference) {
|
||||
// This function is written defensively because I'm not sure about corner cases.
|
||||
// TODO: we can strengthen this if we're sure about the types.
|
||||
const resolved = reference.resolved;
|
||||
if (resolved == null || !Array.isArray(resolved.defs)) {
|
||||
return false;
|
||||
}
|
||||
const def = resolved.defs[0];
|
||||
if (def == null || def.node.init == null) {
|
||||
return false;
|
||||
}
|
||||
// Look for `let stuff = SomeHook();`
|
||||
const init = def.node.init;
|
||||
if (init.callee == null) {
|
||||
return false;
|
||||
}
|
||||
let callee = init.callee;
|
||||
// Step into `= React.something` initializer.
|
||||
if (
|
||||
callee.type === 'MemberExpression' &&
|
||||
callee.object.name === 'React' &&
|
||||
callee.property != null &&
|
||||
!callee.computed
|
||||
) {
|
||||
callee = callee.property;
|
||||
}
|
||||
if (callee.type !== 'Identifier') {
|
||||
return;
|
||||
}
|
||||
const id = def.node.id;
|
||||
if (callee.name === 'useRef' && id.type === 'Identifier') {
|
||||
// useRef() return value is static.
|
||||
return true;
|
||||
} else if (callee.name === 'useState' || callee.name === 'useReducer') {
|
||||
// Only consider second value in initializing tuple static.
|
||||
if (
|
||||
id.type === 'ArrayPattern' &&
|
||||
id.elements.length === 2 &&
|
||||
Array.isArray(reference.resolved.identifiers) &&
|
||||
// Is second tuple value the same reference we're checking?
|
||||
id.elements[1] === reference.resolved.identifiers[0]
|
||||
) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
// By default assume it's dynamic.
|
||||
return false;
|
||||
}
|
||||
|
||||
export default {
|
||||
meta: {
|
||||
fixable: 'code',
|
||||
schema: [
|
||||
{
|
||||
type: 'object',
|
||||
additionalProperties: false,
|
||||
properties: {
|
||||
additionalHooks: {
|
||||
type: 'string',
|
||||
},
|
||||
},
|
||||
},
|
||||
],
|
||||
},
|
||||
create(context) {
|
||||
// Parse the `additionalHooks` regex.
|
||||
const additionalHooks =
|
||||
context.options &&
|
||||
context.options[0] &&
|
||||
context.options[0].additionalHooks
|
||||
? new RegExp(context.options[0].additionalHooks)
|
||||
: undefined;
|
||||
const options = {additionalHooks};
|
||||
|
||||
return {
|
||||
FunctionExpression: visitFunctionExpression,
|
||||
ArrowFunctionExpression: visitFunctionExpression,
|
||||
};
|
||||
|
||||
/**
|
||||
* Visitor for both function expressions and arrow function expressions.
|
||||
*/
|
||||
function visitFunctionExpression(node) {
|
||||
// We only want to lint nodes which are reactive hook callbacks.
|
||||
if (
|
||||
(node.type !== 'FunctionExpression' &&
|
||||
node.type !== 'ArrowFunctionExpression') ||
|
||||
node.parent.type !== 'CallExpression'
|
||||
) {
|
||||
return;
|
||||
}
|
||||
|
||||
const callbackIndex = getReactiveHookCallbackIndex(
|
||||
node.parent.callee,
|
||||
options,
|
||||
);
|
||||
if (node.parent.arguments[callbackIndex] !== node) {
|
||||
return;
|
||||
}
|
||||
|
||||
// Get the reactive hook node.
|
||||
const reactiveHook = node.parent.callee;
|
||||
|
||||
// Get the declared dependencies for this reactive hook. If there is no
|
||||
// second argument then the reactive callback will re-run on every render.
|
||||
// So no need to check for dependency inclusion.
|
||||
const depsIndex = callbackIndex + 1;
|
||||
const declaredDependenciesNode = node.parent.arguments[depsIndex];
|
||||
if (!declaredDependenciesNode) {
|
||||
return;
|
||||
}
|
||||
|
||||
// Get the current scope.
|
||||
const scope = context.getScope();
|
||||
|
||||
// Find all our "pure scopes". On every re-render of a component these
|
||||
// pure scopes may have changes to the variables declared within. So all
|
||||
// variables used in our reactive hook callback but declared in a pure
|
||||
// scope need to be listed as dependencies of our reactive hook callback.
|
||||
//
|
||||
// According to the rules of React you can't read a mutable value in pure
|
||||
// scope. We can't enforce this in a lint so we trust that all variables
|
||||
// declared outside of pure scope are indeed frozen.
|
||||
const pureScopes = new Set();
|
||||
{
|
||||
let currentScope = scope.upper;
|
||||
while (currentScope) {
|
||||
pureScopes.add(currentScope);
|
||||
if (currentScope.type === 'function') {
|
||||
break;
|
||||
}
|
||||
currentScope = currentScope.upper;
|
||||
}
|
||||
// If there is no parent function scope then there are no pure scopes.
|
||||
// The ones we've collected so far are incorrect. So don't continue with
|
||||
// the lint.
|
||||
if (!currentScope) {
|
||||
return;
|
||||
}
|
||||
}
|
||||
|
||||
// Get dependencies from all our resolved references in pure scopes.
|
||||
// Key is dependency string, value is whether it's static.
|
||||
const dependencies = new Map();
|
||||
gatherDependenciesRecursively(scope);
|
||||
|
||||
function gatherDependenciesRecursively(currentScope) {
|
||||
for (const reference of currentScope.references) {
|
||||
// If this reference is not resolved or it is not declared in a pure
|
||||
// scope then we don't care about this reference.
|
||||
if (!reference.resolved) {
|
||||
continue;
|
||||
}
|
||||
if (!pureScopes.has(reference.resolved.scope)) {
|
||||
continue;
|
||||
}
|
||||
// Narrow the scope of a dependency if it is, say, a member expression.
|
||||
// Then normalize the narrowed dependency.
|
||||
|
||||
const referenceNode = fastFindReferenceWithParent(
|
||||
node,
|
||||
reference.identifier,
|
||||
);
|
||||
const dependencyNode = getDependency(referenceNode);
|
||||
const dependency = toPropertyAccessString(dependencyNode);
|
||||
|
||||
// Add the dependency to a map so we can make sure it is referenced
|
||||
// again in our dependencies array. Remember whether it's static.
|
||||
if (!dependencies.has(dependency)) {
|
||||
const isStatic = isDefinitelyStaticDependency(reference);
|
||||
dependencies.set(dependency, isStatic);
|
||||
}
|
||||
}
|
||||
for (const childScope of currentScope.childScopes) {
|
||||
gatherDependenciesRecursively(childScope);
|
||||
}
|
||||
}
|
||||
|
||||
const declaredDependencies = [];
|
||||
if (declaredDependenciesNode.type !== 'ArrayExpression') {
|
||||
// If the declared dependencies are not an array expression then we
|
||||
// can't verify that the user provided the correct dependencies. Tell
|
||||
// the user this in an error.
|
||||
context.report({
|
||||
node: declaredDependenciesNode,
|
||||
message:
|
||||
`React Hook ${context.getSource(reactiveHook)} has a second ` +
|
||||
"argument which is not an array literal. This means we can't " +
|
||||
"statically verify whether you've passed the correct dependencies.",
|
||||
});
|
||||
} else {
|
||||
declaredDependenciesNode.elements.forEach(declaredDependencyNode => {
|
||||
// Skip elided elements.
|
||||
if (declaredDependencyNode === null) {
|
||||
return;
|
||||
}
|
||||
// If we see a spread element then add a special warning.
|
||||
if (declaredDependencyNode.type === 'SpreadElement') {
|
||||
context.report({
|
||||
node: declaredDependencyNode,
|
||||
message:
|
||||
`React Hook ${context.getSource(reactiveHook)} has a spread ` +
|
||||
"element in its dependency array. This means we can't " +
|
||||
"statically verify whether you've passed the " +
|
||||
'correct dependencies.',
|
||||
});
|
||||
return;
|
||||
}
|
||||
// Try to normalize the declared dependency. If we can't then an error
|
||||
// will be thrown. We will catch that error and report an error.
|
||||
let declaredDependency;
|
||||
try {
|
||||
declaredDependency = toPropertyAccessString(declaredDependencyNode);
|
||||
} catch (error) {
|
||||
if (/Unsupported node type/.test(error.message)) {
|
||||
context.report({
|
||||
node: declaredDependencyNode,
|
||||
message:
|
||||
`React Hook ${context.getSource(reactiveHook)} has a ` +
|
||||
`complex expression in the dependency array. ` +
|
||||
'Extract it to a separate variable so it can be statically checked.',
|
||||
});
|
||||
return;
|
||||
} else {
|
||||
throw error;
|
||||
}
|
||||
}
|
||||
// Add the dependency to our declared dependency map.
|
||||
declaredDependencies.push({
|
||||
key: declaredDependency,
|
||||
node: declaredDependencyNode,
|
||||
});
|
||||
});
|
||||
}
|
||||
|
||||
// TODO: we can do a pass at this code and pick more appropriate
|
||||
// data structures to avoid nested loops if we can.
|
||||
let suggestedDependencies = [];
|
||||
let duplicateDependencies = new Set();
|
||||
let unnecessaryDependencies = new Set();
|
||||
let missingDependencies = new Set();
|
||||
let actualDependencies = Array.from(dependencies.keys());
|
||||
|
||||
function satisfies(actualDep, dep) {
|
||||
return actualDep === dep || actualDep.startsWith(dep + '.');
|
||||
}
|
||||
|
||||
// First, ensure what user specified makes sense.
|
||||
declaredDependencies.forEach(({key}) => {
|
||||
if (actualDependencies.some(actualDep => satisfies(actualDep, key))) {
|
||||
// Legit dependency.
|
||||
if (suggestedDependencies.indexOf(key) === -1) {
|
||||
suggestedDependencies.push(key);
|
||||
} else {
|
||||
// Duplicate. Do nothing.
|
||||
duplicateDependencies.add(key);
|
||||
}
|
||||
} else {
|
||||
// Unnecessary dependency. Do nothing.
|
||||
unnecessaryDependencies.add(key);
|
||||
}
|
||||
});
|
||||
|
||||
// Then fill in the missing ones.
|
||||
dependencies.forEach((isStatic, key) => {
|
||||
if (
|
||||
!suggestedDependencies.some(suggestedDep =>
|
||||
satisfies(key, suggestedDep),
|
||||
)
|
||||
) {
|
||||
if (!isStatic) {
|
||||
// Legit missing.
|
||||
suggestedDependencies.push(key);
|
||||
missingDependencies.add(key);
|
||||
}
|
||||
} else {
|
||||
// Already did that. Do nothing.
|
||||
}
|
||||
});
|
||||
|
||||
function areDeclaredDepsAlphabetized() {
|
||||
if (declaredDependencies.length === 0) {
|
||||
return true;
|
||||
}
|
||||
const declaredDepKeys = declaredDependencies.map(dep => dep.key);
|
||||
const sortedDeclaredDepKeys = declaredDepKeys.slice().sort();
|
||||
return declaredDepKeys.join(',') === sortedDeclaredDepKeys.join(',');
|
||||
}
|
||||
|
||||
if (areDeclaredDepsAlphabetized()) {
|
||||
// Alphabetize the autofix, but only if deps were already alphabetized.
|
||||
suggestedDependencies.sort();
|
||||
}
|
||||
|
||||
const problemCount =
|
||||
duplicateDependencies.size +
|
||||
missingDependencies.size +
|
||||
unnecessaryDependencies.size;
|
||||
|
||||
if (problemCount === 0) {
|
||||
return;
|
||||
}
|
||||
|
||||
function getWarningMessage(deps, singlePrefix, label, fixVerb) {
|
||||
if (deps.size === 0) {
|
||||
return null;
|
||||
}
|
||||
return (
|
||||
(deps.size > 1 ? '' : singlePrefix + ' ') +
|
||||
label +
|
||||
' ' +
|
||||
(deps.size > 1 ? 'dependencies' : 'dependency') +
|
||||
': ' +
|
||||
joinEnglish(
|
||||
Array.from(deps)
|
||||
.sort()
|
||||
.map(name => "'" + name + "'"),
|
||||
) +
|
||||
`. Either ${fixVerb} ${
|
||||
deps.size > 1 ? 'them' : 'it'
|
||||
} or remove the dependency array.`
|
||||
);
|
||||
}
|
||||
|
||||
let extraWarning = '';
|
||||
if (unnecessaryDependencies.size > 0) {
|
||||
let badRef = null;
|
||||
Array.from(unnecessaryDependencies.keys()).forEach(key => {
|
||||
if (badRef !== null) {
|
||||
return;
|
||||
}
|
||||
if (key.endsWith('.current')) {
|
||||
badRef = key;
|
||||
}
|
||||
});
|
||||
if (badRef !== null) {
|
||||
extraWarning =
|
||||
` Mutable values like '${badRef}' aren't valid dependencies ` +
|
||||
"because their mutation doesn't re-render the component.";
|
||||
}
|
||||
}
|
||||
|
||||
context.report({
|
||||
node: declaredDependenciesNode,
|
||||
message:
|
||||
`React Hook ${context.getSource(reactiveHook)} has ` +
|
||||
// To avoid a long message, show the next actionable item.
|
||||
(getWarningMessage(missingDependencies, 'a', 'missing', 'include') ||
|
||||
getWarningMessage(
|
||||
unnecessaryDependencies,
|
||||
'an',
|
||||
'unnecessary',
|
||||
'exclude',
|
||||
) ||
|
||||
getWarningMessage(
|
||||
duplicateDependencies,
|
||||
'a',
|
||||
'duplicate',
|
||||
'omit',
|
||||
)) +
|
||||
extraWarning,
|
||||
fix(fixer) {
|
||||
// TODO: consider preserving the comments or formatting?
|
||||
return fixer.replaceText(
|
||||
declaredDependenciesNode,
|
||||
`[${suggestedDependencies.join(', ')}]`,
|
||||
);
|
||||
},
|
||||
});
|
||||
}
|
||||
},
|
||||
};
|
||||
|
||||
/**
|
||||
* Assuming () means the passed/returned node:
|
||||
* (props) => (props)
|
||||
* props.(foo) => (props.foo)
|
||||
* props.foo.(bar) => (props.foo).bar
|
||||
*/
|
||||
function getDependency(node) {
|
||||
if (
|
||||
node.parent.type === 'MemberExpression' &&
|
||||
node.parent.object === node &&
|
||||
node.parent.property.name !== 'current' &&
|
||||
!node.parent.computed &&
|
||||
!(
|
||||
node.parent.parent != null &&
|
||||
node.parent.parent.type === 'CallExpression' &&
|
||||
node.parent.parent.callee === node.parent
|
||||
)
|
||||
) {
|
||||
return node.parent;
|
||||
} else {
|
||||
return node;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Assuming () means the passed node.
|
||||
* (foo) -> 'foo'
|
||||
* foo.(bar) -> 'foo.bar'
|
||||
* foo.bar.(baz) -> 'foo.bar.baz'
|
||||
* Otherwise throw.
|
||||
*/
|
||||
function toPropertyAccessString(node) {
|
||||
if (node.type === 'Identifier') {
|
||||
return node.name;
|
||||
} else if (node.type === 'MemberExpression' && !node.computed) {
|
||||
const object = toPropertyAccessString(node.object);
|
||||
const property = toPropertyAccessString(node.property);
|
||||
return `${object}.${property}`;
|
||||
} else {
|
||||
throw new Error(`Unsupported node type: ${node.type}`);
|
||||
}
|
||||
}
|
||||
|
||||
// What's the index of callback that needs to be analyzed for a given Hook?
|
||||
// -1 if it's not a Hook we care about (e.g. useState).
|
||||
// 0 for useEffect/useMemo/useCallback(fn).
|
||||
// 1 for useImperativeHandle(ref, fn).
|
||||
// For additionally configured Hooks, assume that they're like useEffect (0).
|
||||
function getReactiveHookCallbackIndex(node, options) {
|
||||
let isOnReactObject = false;
|
||||
if (
|
||||
node.type === 'MemberExpression' &&
|
||||
node.object.type === 'Identifier' &&
|
||||
node.object.name === 'React' &&
|
||||
node.property.type === 'Identifier' &&
|
||||
!node.computed
|
||||
) {
|
||||
node = node.property;
|
||||
isOnReactObject = true;
|
||||
}
|
||||
if (node.type !== 'Identifier') {
|
||||
return;
|
||||
}
|
||||
switch (node.name) {
|
||||
case 'useEffect':
|
||||
case 'useLayoutEffect':
|
||||
case 'useCallback':
|
||||
case 'useMemo':
|
||||
// useEffect(fn)
|
||||
return 0;
|
||||
case 'useImperativeHandle':
|
||||
// useImperativeHandle(ref, fn)
|
||||
return 1;
|
||||
default:
|
||||
if (!isOnReactObject && options && options.additionalHooks) {
|
||||
// Allow the user to provide a regular expression which enables the lint to
|
||||
// target custom reactive hooks.
|
||||
let name;
|
||||
try {
|
||||
name = toPropertyAccessString(node);
|
||||
} catch (error) {
|
||||
if (/Unsupported node type/.test(error.message)) {
|
||||
return 0;
|
||||
} else {
|
||||
throw error;
|
||||
}
|
||||
}
|
||||
return options.additionalHooks.test(name) ? 0 : -1;
|
||||
} else {
|
||||
return -1;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* ESLint won't assign node.parent to references from context.getScope()
|
||||
*
|
||||
* So instead we search for the node from an ancestor assigning node.parent
|
||||
* as we go. This mutates the AST.
|
||||
*
|
||||
* This traversal is:
|
||||
* - optimized by only searching nodes with a range surrounding our target node
|
||||
* - agnostic to AST node types, it looks for `{ type: string, ... }`
|
||||
*/
|
||||
function fastFindReferenceWithParent(start, target) {
|
||||
let queue = [start];
|
||||
let item = null;
|
||||
|
||||
while (queue.length) {
|
||||
item = queue.shift();
|
||||
|
||||
if (isSameIdentifier(item, target)) {
|
||||
return item;
|
||||
}
|
||||
|
||||
if (!isAncestorNodeOf(item, target)) {
|
||||
continue;
|
||||
}
|
||||
|
||||
for (let [key, value] of Object.entries(item)) {
|
||||
if (key === 'parent') {
|
||||
continue;
|
||||
}
|
||||
if (isNodeLike(value)) {
|
||||
value.parent = item;
|
||||
queue.push(value);
|
||||
} else if (Array.isArray(value)) {
|
||||
value.forEach(val => {
|
||||
if (isNodeLike(val)) {
|
||||
val.parent = item;
|
||||
queue.push(val);
|
||||
}
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return null;
|
||||
}
|
||||
|
||||
function joinEnglish(arr) {
|
||||
let s = '';
|
||||
for (let i = 0; i < arr.length; i++) {
|
||||
s += arr[i];
|
||||
if (i === 0 && arr.length === 2) {
|
||||
s += ' and ';
|
||||
} else if (i === arr.length - 2 && arr.length > 2) {
|
||||
s += ', and ';
|
||||
} else if (i < arr.length - 1) {
|
||||
s += ', ';
|
||||
}
|
||||
}
|
||||
return s;
|
||||
}
|
||||
|
||||
function isNodeLike(val) {
|
||||
return (
|
||||
typeof val === 'object' &&
|
||||
val !== null &&
|
||||
!Array.isArray(val) &&
|
||||
typeof val.type === 'string'
|
||||
);
|
||||
}
|
||||
|
||||
function isSameIdentifier(a, b) {
|
||||
return (
|
||||
a.type === 'Identifier' &&
|
||||
a.name === b.name &&
|
||||
a.range[0] === b.range[0] &&
|
||||
a.range[1] === b.range[1]
|
||||
);
|
||||
}
|
||||
|
||||
function isAncestorNodeOf(a, b) {
|
||||
return a.range[0] <= b.range[0] && a.range[1] >= b.range[1];
|
||||
}
|
||||
|
|
@ -8,7 +8,9 @@
|
|||
'use strict';
|
||||
|
||||
import RuleOfHooks from './RulesOfHooks';
|
||||
import ExhaustiveDeps from './ExhaustiveDeps';
|
||||
|
||||
export const rules = {
|
||||
'rules-of-hooks': RuleOfHooks,
|
||||
'exhaustive-deps': ExhaustiveDeps,
|
||||
};
|
||||
|
|
|
|||
Loading…
Reference in New Issue