Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix instances of function declaration after return #19733

Merged
merged 2 commits into from
Sep 1, 2020
Merged

Fix instances of function declaration after return #19733

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ed2b15e
Add ESLint plugin to check for any function declare after return
bhumijgupta Sep 1, 2020
d21aab4
Refactor code to move function declarations before return and fix fai…
bhumijgupta Sep 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion .eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand Down Expand Up @@ -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,
Expand Down
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
295 changes: 147 additions & 148 deletions packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand Down Expand Up @@ -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,
};
},
};

Expand Down
3 changes: 1 addition & 2 deletions ...ages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/UIManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = '';
Expand All @@ -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) {
Expand Down
13 changes: 6 additions & 7 deletions packages/react-native-renderer/src/legacy-events/SyntheticEvent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -296,6 +289,12 @@ function getPooledWarningPropertyDefinition(propName, getVal) {
);
}
}
const isFunction = typeof getVal === 'function';
return {
configurable: true,
set: set,
get: get,
};
}

function createOrGetPooledEvent(
Expand Down
5 changes: 5 additions & 0 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down