Skip to content

Commit

Permalink
Refactor code to move function declarations before return and fix fai…
Browse files Browse the repository at this point in the history 
…ling lint

Signed-off-by: bhumijgupta <bhumijgupta@gmail.com>
  • Loading branch information
@bhumijgupta
bhumijgupta committed Sep 1, 2020
1 parent ed2b15e commit d21aab4
Show file tree
Hide file tree
Showing 3 changed files with 154 additions and 157 deletions.
295 changes: 147 additions & 148 deletions packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
View file
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
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
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

0 comments on commit d21aab4

Please sign in to comment.