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.

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

[eslint] Check forwardRef callbacks #17255

Merged
merged 5 commits into from
Nov 17, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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')],
},
Copy link
Collaborator

@gaearon gaearon Nov 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also add tests verifying how we handle React.whatever(props => ...) or whatever(props => ...)? i.e. the case where isReactFunction returns false.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added some more tests. The existing behaviour is to skip checking unnamed callbacks - there's a regression test in place for this.

{
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