Skip to content

Commit

Permalink
[eslint] Check forwardRef callbacks (#17255)
Browse files Browse the repository at this point in the history
* [eslint] Check forwardRef callbacks (#17220)

* [eslint] Make tests more realistic (#17220)

* [eslint] Check anonymous callback of React.memo for rules-of-hooks (#17220)

* [eslint] Add tests for callbacks not known to be components (#17220)

* [eslint] Correct comments and add another test (#17220)
  • Loading branch information
dprgarner authored and gaearon committed Nov 17, 2019
1 parent 2586303 commit a807c30
Show file tree
Hide file tree
Showing 2 changed files with 174 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,46 @@ const tests = {
return useHook1(useHook2());
}
`,
`
// Valid because hooks can be used in anonymous arrow-function arguments
// to forwardRef.
const FancyButton = React.forwardRef((props, ref) => {
useHook();
return <button {...props} ref={ref} />
});
`,
`
// Valid because hooks can be used in anonymous function arguments to
// forwardRef.
const FancyButton = React.forwardRef(function (props, ref) {
useHook();
return <button {...props} ref={ref} />
});
`,
`
// Valid because hooks can be used in anonymous function arguments to
// forwardRef.
const FancyButton = forwardRef(function (props, ref) {
useHook();
return <button {...props} ref={ref} />
});
`,
`
// Valid because hooks can be used in anonymous function arguments to
// React.memo.
const MemoizedFunction = React.memo(props => {
useHook();
return <button {...props} />
});
`,
`
// Valid because hooks can be used in anonymous function arguments to
// memo.
const MemoizedFunction = memo(function (props) {
useHook();
return <button {...props} />
});
`,
`
// Valid because classes can call functions.
// We don't consider these to be hooks.
Expand Down Expand Up @@ -262,6 +302,24 @@ const tests = {
});
}
`,
`
// This is valid because "use"-prefixed functions called in
// unnamed function arguments are not assumed to be hooks.
React.unknownFunction((foo, bar) => {
if (foo) {
useNotAHook(bar)
}
});
`,
`
// This is valid because "use"-prefixed functions called in
// unnamed function arguments are not assumed to be hooks.
unknownFunction(function(foo, bar) {
if (foo) {
useNotAHook(bar)
}
});
`,
`
// Regression test for incorrectly flagged valid code.
function RegressionTest() {
Expand Down Expand Up @@ -437,6 +495,32 @@ const tests = {
`,
errors: [genericError('useHookInsideCallback')],
},
{
code: `
// Invalid because it's a common misunderstanding.
// We *could* make it valid but the runtime error could be confusing.
const ComponentWithHookInsideCallback = React.forwardRef((props, ref) => {
useEffect(() => {
useHookInsideCallback();
});
return <button {...props} ref={ref} />
});
`,
errors: [genericError('useHookInsideCallback')],
},
{
code: `
// Invalid because it's a common misunderstanding.
// We *could* make it valid but the runtime error could be confusing.
const ComponentWithHookInsideCallback = React.memo(props => {
useEffect(() => {
useHookInsideCallback();
});
return <button {...props} />
});
`,
errors: [genericError('useHookInsideCallback')],
},
{
code: `
// Invalid because it's a common misunderstanding.
Expand Down Expand Up @@ -695,6 +779,55 @@ const tests = {
// conditionalError('useState'),
],
},
{
code: `
// Invalid because it's dangerous and might not warn otherwise.
// This *must* be invalid.
const FancyButton = React.forwardRef((props, ref) => {
if (props.fancy) {
useCustomHook();
}
return <button ref={ref}>{props.children}</button>;
});
`,
errors: [conditionalError('useCustomHook')],
},
{
code: `
// Invalid because it's dangerous and might not warn otherwise.
// This *must* be invalid.
const FancyButton = forwardRef(function(props, ref) {
if (props.fancy) {
useCustomHook();
}
return <button ref={ref}>{props.children}</button>;
});
`,
errors: [conditionalError('useCustomHook')],
},
{
code: `
// Invalid because it's dangerous and might not warn otherwise.
// This *must* be invalid.
const MemoizedButton = memo(function(props) {
if (props.fancy) {
useCustomHook();
}
return <button>{props.children}</button>;
});
`,
errors: [conditionalError('useCustomHook')],
},
{
code: `
// This is invalid because "use"-prefixed functions used in named
// functions are assumed to be hooks.
React.unknownFunction(function notAComponent(foo, bar) {
useProbablyAHook(bar)
});
`,
errors: [functionError('useProbablyAHook', 'notAComponent')],
},
{
code: `
// Invalid because it's dangerous.
Expand Down
43 changes: 41 additions & 2 deletions packages/eslint-plugin-react-hooks/src/RulesOfHooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,41 @@ function isComponentName(node) {
}
}

function isReactFunction(node, functionName) {
return (
node.name === functionName ||
(node.type === 'MemberExpression' &&
node.object.name === 'React' &&
node.property.name === functionName)
);
}

/**
* Checks if the node is a callback argument of forwardRef. This render function
* should follow the rules of hooks.
*/

function isForwardRefCallback(node) {
return !!(
node.parent &&
node.parent.callee &&
isReactFunction(node.parent.callee, 'forwardRef')
);
}

/**
* Checks if the node is a callback argument of React.memo. This anonymous
* functional component should follow the rules of hooks.
*/

function isMemoCallback(node) {
return !!(
node.parent &&
node.parent.callee &&
isReactFunction(node.parent.callee, 'memo')
);
}

function isInsideComponentOrHook(node) {
while (node) {
const functionName = getFunctionName(node);
Expand All @@ -62,6 +97,9 @@ function isInsideComponentOrHook(node) {
return true;
}
}
if (isForwardRefCallback(node) || isMemoCallback(node)) {
return true;
}
node = node.parent;
}
return false;
Expand Down Expand Up @@ -290,7 +328,8 @@ export default {
// `undefined` then we know either that we have an anonymous function
// expression or our code path is not in a function. In both cases we
// will want to error since neither are React function components or
// hook functions.
// hook functions - unless it is an anonymous function argument to
// forwardRef or memo.
const codePathFunctionName = getFunctionName(codePathNode);

// This is a valid code path for React hooks if we are directly in a React
Expand All @@ -301,7 +340,7 @@ export default {
const isDirectlyInsideComponentOrHook = codePathFunctionName
? isComponentName(codePathFunctionName) ||
isHook(codePathFunctionName)
: false;
: isForwardRefCallback(codePathNode) || isMemoCallback(codePathNode);

// Compute the earliest finalizer level using information from the
// cache. We expect all reachable final segments to have a cache entry
Expand Down

0 comments on commit a807c30

Please sign in to comment.