From 53e622ca7f643cc9d18c9c4896b68ab14549e292 Mon Sep 17 00:00:00 2001 From: Bhumij Gupta Date: Tue, 1 Sep 2020 18:25:10 +0530 Subject: [PATCH] Fix instances of function declaration after return (#19733) * Add ESLint plugin to check for any function declare after return * Refactor code to move function declarations before return and fix failing lint --- .eslintrc.js | 11 +- package.json | 1 + .../src/ExhaustiveDeps.js | 295 +++++++++--------- .../Libraries/ReactPrivate/UIManager.js | 3 +- .../src/legacy-events/SyntheticEvent.js | 13 +- yarn.lock | 5 + 6 files changed, 170 insertions(+), 158 deletions(-) diff --git a/.eslintrc.js b/.eslintrc.js index b6d07a79e6601..6eb0eefbda3c3 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -16,7 +16,13 @@ module.exports = { // Stop ESLint from looking for a configuration file in parent folders root: true, - plugins: ['jest', 'no-for-of-loops', 'react', 'react-internal'], + plugins: [ + 'jest', + 'no-for-of-loops', + 'no-function-declare-after-return', + 'react', + 'react-internal', + ], parser: 'babel-eslint', parserOptions: { @@ -91,6 +97,9 @@ module.exports = { // You can disable this rule for code that isn't shipped (e.g. build scripts and tests). 'no-for-of-loops/no-for-of-loops': ERROR, + // Prevent function declarations after return statements + 'no-function-declare-after-return/no-function-declare-after-return': ERROR, + // CUSTOM RULES // the second argument of warning/invariant should be a literal string 'react-internal/no-primitive-constructors': ERROR, diff --git a/package.json b/package.json index 90abea8e5f347..b093e33e90023 100644 --- a/package.json +++ b/package.json @@ -54,6 +54,7 @@ "eslint-plugin-flowtype": "^2.25.0", "eslint-plugin-jest": "^22.15.0", "eslint-plugin-no-for-of-loops": "^1.0.0", + "eslint-plugin-no-function-declare-after-return": "^1.0.0", "eslint-plugin-react": "^6.7.1", "eslint-plugin-react-internal": "link:./scripts/eslint-rules", "fbjs-scripts": "1.2.0", diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index e018cdf843745..9899abc0c7d49 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -86,154 +86,6 @@ export default { return result; }; } - - return { - CallExpression: visitCallExpression, - }; - - function visitCallExpression(node) { - const callbackIndex = getReactiveHookCallbackIndex(node.callee, options); - if (callbackIndex === -1) { - // Not a React Hook call that needs deps. - return; - } - const callback = node.arguments[callbackIndex]; - const reactiveHook = node.callee; - const reactiveHookName = getNodeWithoutReactNamespace(reactiveHook).name; - const declaredDependenciesNode = node.arguments[callbackIndex + 1]; - const isEffect = /Effect($|[^a-z])/g.test(reactiveHookName); - - // Check 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. - if (!declaredDependenciesNode && !isEffect) { - // These are only used for optimization. - if ( - reactiveHookName === 'useMemo' || - reactiveHookName === 'useCallback' - ) { - // TODO: Can this have a suggestion? - reportProblem({ - node: reactiveHook, - message: - `React Hook ${reactiveHookName} does nothing when called with ` + - `only one argument. Did you forget to pass an array of ` + - `dependencies?`, - }); - } - return; - } - - switch (callback.type) { - case 'FunctionExpression': - case 'ArrowFunctionExpression': - visitFunctionWithDependencies( - callback, - declaredDependenciesNode, - reactiveHook, - reactiveHookName, - isEffect, - ); - return; // Handled - case 'Identifier': - if (!declaredDependenciesNode) { - // No deps, no problems. - return; // Handled - } - // The function passed as a callback is not written inline. - // But perhaps it's in the dependencies array? - if ( - declaredDependenciesNode.elements && - declaredDependenciesNode.elements.some( - el => el && el.type === 'Identifier' && el.name === callback.name, - ) - ) { - // If it's already in the list of deps, we don't care because - // this is valid regardless. - return; // Handled - } - // We'll do our best effort to find it, complain otherwise. - const variable = context.getScope().set.get(callback.name); - if (variable == null || variable.defs == null) { - // If it's not in scope, we don't care. - return; // Handled - } - // The function passed as a callback is not written inline. - // But it's defined somewhere in the render scope. - // We'll do our best effort to find and check it, complain otherwise. - const def = variable.defs[0]; - if (!def || !def.node) { - break; // Unhandled - } - if (def.type !== 'Variable' && def.type !== 'FunctionName') { - // Parameter or an unusual pattern. Bail out. - break; // Unhandled - } - switch (def.node.type) { - case 'FunctionDeclaration': - // useEffect(() => { ... }, []); - visitFunctionWithDependencies( - def.node, - declaredDependenciesNode, - reactiveHook, - reactiveHookName, - isEffect, - ); - return; // Handled - case 'VariableDeclarator': - const init = def.node.init; - if (!init) { - break; // Unhandled - } - switch (init.type) { - // const effectBody = () => {...}; - // useEffect(effectBody, []); - case 'ArrowFunctionExpression': - case 'FunctionExpression': - // We can inspect this function as if it were inline. - visitFunctionWithDependencies( - init, - declaredDependenciesNode, - reactiveHook, - reactiveHookName, - isEffect, - ); - return; // Handled - } - break; // Unhandled - } - break; // Unhandled - default: - // useEffect(generateEffectBody(), []); - reportProblem({ - node: reactiveHook, - message: - `React Hook ${reactiveHookName} received a function whose dependencies ` + - `are unknown. Pass an inline function instead.`, - }); - return; // Handled - } - - // Something unusual. Fall back to suggesting to add the body itself as a dep. - reportProblem({ - node: reactiveHook, - message: - `React Hook ${reactiveHookName} has a missing dependency: '${callback.name}'. ` + - `Either include it or remove the dependency array.`, - suggest: [ - { - desc: `Update the dependencies array to be: [${callback.name}]`, - fix(fixer) { - return fixer.replaceText( - declaredDependenciesNode, - `[${callback.name}]`, - ); - }, - }, - ], - }); - } - /** * Visitor for both function expressions and arrow function expressions. */ @@ -1251,6 +1103,153 @@ export default { ], }); } + + function visitCallExpression(node) { + const callbackIndex = getReactiveHookCallbackIndex(node.callee, options); + if (callbackIndex === -1) { + // Not a React Hook call that needs deps. + return; + } + const callback = node.arguments[callbackIndex]; + const reactiveHook = node.callee; + const reactiveHookName = getNodeWithoutReactNamespace(reactiveHook).name; + const declaredDependenciesNode = node.arguments[callbackIndex + 1]; + const isEffect = /Effect($|[^a-z])/g.test(reactiveHookName); + + // Check 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. + if (!declaredDependenciesNode && !isEffect) { + // These are only used for optimization. + if ( + reactiveHookName === 'useMemo' || + reactiveHookName === 'useCallback' + ) { + // TODO: Can this have a suggestion? + reportProblem({ + node: reactiveHook, + message: + `React Hook ${reactiveHookName} does nothing when called with ` + + `only one argument. Did you forget to pass an array of ` + + `dependencies?`, + }); + } + return; + } + + switch (callback.type) { + case 'FunctionExpression': + case 'ArrowFunctionExpression': + visitFunctionWithDependencies( + callback, + declaredDependenciesNode, + reactiveHook, + reactiveHookName, + isEffect, + ); + return; // Handled + case 'Identifier': + if (!declaredDependenciesNode) { + // No deps, no problems. + return; // Handled + } + // The function passed as a callback is not written inline. + // But perhaps it's in the dependencies array? + if ( + declaredDependenciesNode.elements && + declaredDependenciesNode.elements.some( + el => el && el.type === 'Identifier' && el.name === callback.name, + ) + ) { + // If it's already in the list of deps, we don't care because + // this is valid regardless. + return; // Handled + } + // We'll do our best effort to find it, complain otherwise. + const variable = context.getScope().set.get(callback.name); + if (variable == null || variable.defs == null) { + // If it's not in scope, we don't care. + return; // Handled + } + // The function passed as a callback is not written inline. + // But it's defined somewhere in the render scope. + // We'll do our best effort to find and check it, complain otherwise. + const def = variable.defs[0]; + if (!def || !def.node) { + break; // Unhandled + } + if (def.type !== 'Variable' && def.type !== 'FunctionName') { + // Parameter or an unusual pattern. Bail out. + break; // Unhandled + } + switch (def.node.type) { + case 'FunctionDeclaration': + // useEffect(() => { ... }, []); + visitFunctionWithDependencies( + def.node, + declaredDependenciesNode, + reactiveHook, + reactiveHookName, + isEffect, + ); + return; // Handled + case 'VariableDeclarator': + const init = def.node.init; + if (!init) { + break; // Unhandled + } + switch (init.type) { + // const effectBody = () => {...}; + // useEffect(effectBody, []); + case 'ArrowFunctionExpression': + case 'FunctionExpression': + // We can inspect this function as if it were inline. + visitFunctionWithDependencies( + init, + declaredDependenciesNode, + reactiveHook, + reactiveHookName, + isEffect, + ); + return; // Handled + } + break; // Unhandled + } + break; // Unhandled + default: + // useEffect(generateEffectBody(), []); + reportProblem({ + node: reactiveHook, + message: + `React Hook ${reactiveHookName} received a function whose dependencies ` + + `are unknown. Pass an inline function instead.`, + }); + return; // Handled + } + + // Something unusual. Fall back to suggesting to add the body itself as a dep. + reportProblem({ + node: reactiveHook, + message: + `React Hook ${reactiveHookName} has a missing dependency: '${callback.name}'. ` + + `Either include it or remove the dependency array.`, + suggest: [ + { + desc: `Update the dependencies array to be: [${callback.name}]`, + fix(fixer) { + return fixer.replaceText( + declaredDependenciesNode, + `[${callback.name}]`, + ); + }, + }, + ], + }); + } + + return { + CallExpression: visitCallExpression, + }; }, }; diff --git a/packages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/UIManager.js b/packages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/UIManager.js index f155f6af9b186..f5245f74fa9c9 100644 --- a/packages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/UIManager.js +++ b/packages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/UIManager.js @@ -60,8 +60,6 @@ function removeChild(parent, child) { const RCTUIManager = { __dumpHierarchyForJestTestsOnly: function() { - return roots.map(tag => dumpSubtree(tag, 0)).join('\n'); - function dumpSubtree(tag, indent) { const info = views.get(tag); let out = ''; @@ -73,6 +71,7 @@ const RCTUIManager = { } return out; } + return roots.map(tag => dumpSubtree(tag, 0)).join('\n'); }, clearJSResponder: jest.fn(), createView: jest.fn(function createView(reactTag, viewName, rootTag, props) { diff --git a/packages/react-native-renderer/src/legacy-events/SyntheticEvent.js b/packages/react-native-renderer/src/legacy-events/SyntheticEvent.js index a439f8eaa4367..329d89ec9f7d2 100644 --- a/packages/react-native-renderer/src/legacy-events/SyntheticEvent.js +++ b/packages/react-native-renderer/src/legacy-events/SyntheticEvent.js @@ -259,13 +259,6 @@ addEventPoolingTo(SyntheticEvent); * @return {object} defineProperty object */ function getPooledWarningPropertyDefinition(propName, getVal) { - const isFunction = typeof getVal === 'function'; - return { - configurable: true, - set: set, - get: get, - }; - function set(val) { const action = isFunction ? 'setting the method' : 'setting the property'; warn(action, 'This is effectively a no-op'); @@ -296,6 +289,12 @@ function getPooledWarningPropertyDefinition(propName, getVal) { ); } } + const isFunction = typeof getVal === 'function'; + return { + configurable: true, + set: set, + get: get, + }; } function createOrGetPooledEvent( diff --git a/yarn.lock b/yarn.lock index 09440837648bd..cc19b161d748d 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5407,6 +5407,11 @@ eslint-plugin-no-for-of-loops@^1.0.0: resolved "https://registry.yarnpkg.com/eslint-plugin-no-for-of-loops/-/eslint-plugin-no-for-of-loops-1.0.1.tgz#2ee3732d50c642b8d1bfacedbfe3b28f86222369" integrity sha512-uCotzBHt2W+HbLw2srRmqDJHOPbJGzeVLstKh8YyxS3ppduq2P50qdpJfHKoD+UGbnqA/zhy8NRgPH6p0y8bnA== +eslint-plugin-no-function-declare-after-return@^1.0.0: + version "1.0.0" + resolved "https://registry.yarnpkg.com/eslint-plugin-no-function-declare-after-return/-/eslint-plugin-no-function-declare-after-return-1.0.0.tgz#ddf01d71d27b37ced61ccb295c6ac0708d87b209" + integrity sha512-/w6tuSK4kYSwHSlPtf36u/rQOG8YVPgl8a0bh4rV/ElZNaUGYOquY/hp4jaCnEmPta0aTpAkFEmSi6ZTd7wCEQ== + eslint-plugin-no-unsanitized@3.1.2: version "3.1.2" resolved "https://registry.yarnpkg.com/eslint-plugin-no-unsanitized/-/eslint-plugin-no-unsanitized-3.1.2.tgz#a54724e0b81d43279bb1f8f5e1d82c97da78c858"