From 1960131f11196325ff47458355d25071049aaa7a Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 31 Mar 2020 11:43:01 +0100 Subject: [PATCH] Add opt-in support for dangerous autofix (#18437) --- .../ESLintRuleExhaustiveDeps-test.js | 28 ++++++++++ .../src/ExhaustiveDeps.js | 56 ++++++++++++++----- 2 files changed, 69 insertions(+), 15 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index 7c4639607b..57ce80b99f 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -6336,6 +6336,34 @@ const tests = { }, ], }, + { + code: normalizeIndent` + function MyComponent() { + const local = {}; + useEffect(() => { + console.log(local); + }, []); + } + `, + // Dangerous autofix is enabled due to the option: + output: normalizeIndent` + function MyComponent() { + const local = {}; + useEffect(() => { + console.log(local); + }, [local]); + } + `, + errors: [ + { + message: + "React Hook useEffect has a missing dependency: 'local'. " + + 'Either include it or remove the dependency array.', + }, + ], + // Keep this until major IDEs and VS Code FB ESLint plugin support Suggestions API. + options: [{enableDangerousAutofixThisMayCauseInfiniteLoops: true}], + }, ], }; diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index 4482d119b9..b4bc461913 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -11,14 +11,19 @@ export default { meta: { + fixable: 'code', schema: [ { type: 'object', additionalProperties: false, + enableDangerousAutofixThisMayCauseInfiniteLoops: false, properties: { additionalHooks: { type: 'string', }, + enableDangerousAutofixThisMayCauseInfiniteLoops: { + type: 'boolean', + }, }, }, ], @@ -31,7 +36,28 @@ export default { context.options[0].additionalHooks ? new RegExp(context.options[0].additionalHooks) : undefined; - const options = {additionalHooks}; + + const enableDangerousAutofixThisMayCauseInfiniteLoops = + (context.options && + context.options[0] && + context.options[0].enableDangerousAutofixThisMayCauseInfiniteLoops) || + false; + + const options = { + additionalHooks, + enableDangerousAutofixThisMayCauseInfiniteLoops, + }; + + function reportProblem(problem) { + if (enableDangerousAutofixThisMayCauseInfiniteLoops) { + // Used to enable legacy behavior. Dangerous. + // Keep this as an option until major IDEs upgrade (including VSCode FB ESLint extension). + if (Array.isArray(problem.suggest) && problem.suggest.length > 0) { + problem.fix = problem.suggest[0].fix; + } + } + context.report(problem); + } const scopeManager = context.getSourceCode().scopeManager; @@ -140,7 +166,7 @@ export default { break; // Unhandled default: // useEffect(generateEffectBody(), []); - context.report({ + reportProblem({ node: reactiveHook, message: `React Hook ${reactiveHookName} received a function whose dependencies ` + @@ -150,7 +176,7 @@ export default { } // Something unusual. Fall back to suggesting to add the body itself as a dep. - context.report({ + reportProblem({ node: reactiveHook, message: `React Hook ${reactiveHookName} has a missing dependency: '${callback.name}'. ` + @@ -190,7 +216,7 @@ export default { reactiveHookName === 'useCallback' ) { // TODO: Can this have a suggestion? - context.report({ + reportProblem({ node: reactiveHook, message: `React Hook ${reactiveHookName} does nothing when called with ` + @@ -202,7 +228,7 @@ export default { } if (isEffect && node.async) { - context.report({ + reportProblem({ node: node, message: `Effect callbacks are synchronous to prevent race conditions. ` + @@ -557,7 +583,7 @@ export default { if (foundCurrentAssignment) { return; } - context.report({ + reportProblem({ node: dependencyNode.parent.property, message: `The ref value '${dependency}.current' will likely have ` + @@ -577,7 +603,7 @@ export default { return; } staleAssignments.add(key); - context.report({ + reportProblem({ node: writeExpr, message: `Assignments to the '${key}' variable from inside React Hook ` + @@ -645,7 +671,7 @@ export default { externalDependencies: new Set(), isEffect: true, }); - context.report({ + reportProblem({ node: reactiveHook, message: `React Hook ${reactiveHookName} contains a call to '${setStateInsideEffectWithoutDeps}'. ` + @@ -677,7 +703,7 @@ export default { // 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({ + reportProblem({ node: declaredDependenciesNode, message: `React Hook ${context.getSource(reactiveHook)} was passed a ` + @@ -693,7 +719,7 @@ export default { } // If we see a spread element then add a special warning. if (declaredDependencyNode.type === 'SpreadElement') { - context.report({ + reportProblem({ node: declaredDependencyNode, message: `React Hook ${context.getSource(reactiveHook)} has a spread ` + @@ -712,7 +738,7 @@ export default { if (/Unsupported node type/.test(error.message)) { if (declaredDependencyNode.type === 'Literal') { if (dependencies.has(declaredDependencyNode.value)) { - context.report({ + reportProblem({ node: declaredDependencyNode, message: `The ${declaredDependencyNode.raw} literal is not a valid dependency ` + @@ -720,7 +746,7 @@ export default { `Did you mean to include ${declaredDependencyNode.value} in the array instead?`, }); } else { - context.report({ + reportProblem({ node: declaredDependencyNode, message: `The ${declaredDependencyNode.raw} literal is not a valid dependency ` + @@ -728,7 +754,7 @@ export default { }); } } else { - context.report({ + reportProblem({ node: declaredDependencyNode, message: `React Hook ${context.getSource(reactiveHook)} has a ` + @@ -828,7 +854,7 @@ export default { } // TODO: What if the function needs to change on every render anyway? // Should we suggest removing effect deps as an appropriate fix too? - context.report({ + reportProblem({ // TODO: Why not report this at the dependency site? node: fn.node, message, @@ -1099,7 +1125,7 @@ export default { } } - context.report({ + reportProblem({ node: declaredDependenciesNode, message: `React Hook ${context.getSource(reactiveHook)} has ` +