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
This commit is contained in:
parent
b7d18c4daf
commit
53e622ca7f
11
.eslintrc.js
11
.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,
|
||||
|
|
|
@ -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",
|
||||
|
|
|
@ -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,
|
||||
};
|
||||
},
|
||||
};
|
||||
|
||||
|
|
|
@ -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) {
|
||||
|
|
|
@ -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(
|
||||
|
|
|
@ -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"
|
||||
|
|
Loading…
Reference in New Issue