Skip to content

[eslint] Do not allow useEffectEvent fns to be called in arbitrary closures #33544

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+78 −70
Open

[eslint] Do not allow useEffectEvent fns to be called in arbitrary closures #33544

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
112 changes: 56 additions & 56 deletions packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1343,33 +1343,6 @@ if (__EXPERIMENTAL__) {
}
`,
},
{
code: normalizeIndent`
// Valid because functions created with useEffectEvent can be called in closures.
function MyComponent({ theme }) {
const onClick = useEffectEvent(() => {
showNotification(theme);
});
return <Child onClick={() => onClick()}></Child>;
}
`,
},
{
code: normalizeIndent`
// Valid because functions created with useEffectEvent can be called in closures.
function MyComponent({ theme }) {
const onClick = useEffectEvent(() => {
showNotification(theme);
});
const onClick2 = () => { onClick() };
const onClick3 = useCallback(() => onClick(), []);
return <>
<Child onClick={onClick2}></Child>
<Child onClick={onClick3}></Child>
</>;
}
`,
},
{
code: normalizeIndent`
// Valid because functions created with useEffectEvent can be passed by reference in useEffect
Expand All @@ -1380,47 +1353,39 @@ if (__EXPERIMENTAL__) {
});
const onClick2 = useEffectEvent(() => {
debounce(onClick);
debounce(() => onClick());
debounce(() => { onClick() });
deboucne(() => debounce(onClick));
});
useEffect(() => {
let id = setInterval(onClick, 100);
let id = setInterval(() => onClick(), 100);
return () => clearInterval(onClick);
}, []);
return <Child onClick={() => onClick2()} />
return null;
}
`,
},
{
code: normalizeIndent`
const MyComponent = ({theme}) => {
const onClick = useEffectEvent(() => {
showNotification(theme);
});
return <Child onClick={() => onClick()}></Child>;
};
`,
},
{
code: normalizeIndent`
function MyComponent({ theme }) {
const notificationService = useNotifications();
const showNotification = useEffectEvent((text) => {
notificationService.notify(theme, text);
useEffect(() => {
onClick();
});
const onClick = useEffectEvent((text) => {
showNotification(text);
const onClick = useEffectEvent(() => {
showNotification(theme);
});
return <Child onClick={(text) => onClick(text)} />
}
`,
},
{
code: normalizeIndent`
function MyComponent({ theme }) {
useEffect(() => {
onClick();
const onEvent = useEffectEvent((text) => {
console.log(text);
});
const onClick = useEffectEvent(() => {
showNotification(theme);

useEffect(() => {
onEvent('Hello world');
});
}
`,
Expand All @@ -1437,7 +1402,7 @@ if (__EXPERIMENTAL__) {
return <Child onClick={onClick}></Child>;
}
`,
errors: [useEffectEventError('onClick')],
errors: [useEffectEventError('onClick', false)],
},
{
code: normalizeIndent`
Expand All @@ -1456,8 +1421,23 @@ if (__EXPERIMENTAL__) {
});
return <Child onClick={() => onClick()} />
}

// The useEffectEvent function shares an identifier name with the above
function MyLastComponent({theme}) {
const onClick = useEffectEvent(() => {
showNotification(theme)
});
useEffect(() => {
onClick(); // No error here, errors on all other uses
onClick;
})
return <Child />
}
`,
errors: [{...useEffectEventError('onClick'), line: 7}],
errors: [
{...useEffectEventError('onClick', false), line: 7},
{...useEffectEventError('onClick', true), line: 15},
],
},
{
code: normalizeIndent`
Expand All @@ -1468,7 +1448,7 @@ if (__EXPERIMENTAL__) {
return <Child onClick={onClick}></Child>;
}
`,
errors: [useEffectEventError('onClick')],
errors: [useEffectEventError('onClick', false)],
},
{
code: normalizeIndent`
Expand All @@ -1481,7 +1461,7 @@ if (__EXPERIMENTAL__) {
return <Bar onClick={foo} />
}
`,
errors: [{...useEffectEventError('onClick'), line: 7}],
errors: [{...useEffectEventError('onClick', false), line: 7}],
},
{
code: normalizeIndent`
Expand All @@ -1497,7 +1477,27 @@ if (__EXPERIMENTAL__) {
return <Child onClick={onClick} />
}
`,
errors: [useEffectEventError('onClick')],
errors: [useEffectEventError('onClick', false)],
},
{
code: normalizeIndent`
// Invalid because functions created with useEffectEvent cannot be called in arbitrary closures.
function MyComponent({ theme }) {
const onClick = useEffectEvent(() => {
showNotification(theme);
});
const onClick2 = () => { onClick() };
const onClick3 = useCallback(() => onClick(), []);
return <>
<Child onClick={onClick2}></Child>
<Child onClick={onClick3}></Child>
</>;
}
`,
errors: [
useEffectEventError('onClick', true),
useEffectEventError('onClick', true),
],
},
];
}
Expand Down Expand Up @@ -1559,11 +1559,11 @@ function classError(hook) {
};
}

function useEffectEventError(fn) {
function useEffectEventError(fn, called) {
return {
message:
`\`${fn}\` is a function created with React Hook "useEffectEvent", and can only be called from ` +
'the same component. They cannot be assigned to variables or passed down.',
`the same component.${called ? '' : ' They cannot be assigned to variables or passed down.'}`,
};
}

Expand Down
36 changes: 22 additions & 14 deletions packages/eslint-plugin-react-hooks/src/rules/RulesOfHooks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -541,7 +541,9 @@ const rule = {
context.report({
node: hook,
message:
`React Hook "${getSourceCode().getText(hook)}" may be executed ` +
`React Hook "${getSourceCode().getText(
hook,
)}" may be executed ` +
'more than once. Possibly because it is called in a loop. ' +
'React Hooks must be called in the exact same order in ' +
'every component render.',
Expand Down Expand Up @@ -596,7 +598,9 @@ const rule = {
) {
// Custom message for hooks inside a class
const message =
`React Hook "${getSourceCode().getText(hook)}" cannot be called ` +
`React Hook "${getSourceCode().getText(
hook,
)}" cannot be called ` +
'in a class component. React Hooks must be called in a ' +
'React function component or a custom React Hook function.';
context.report({node: hook, message});
Expand All @@ -613,7 +617,9 @@ const rule = {
} else if (codePathNode.type === 'Program') {
// These are dangerous if you have inline requires enabled.
const message =
`React Hook "${getSourceCode().getText(hook)}" cannot be called ` +
`React Hook "${getSourceCode().getText(
hook,
)}" cannot be called ` +
'at the top level. React Hooks must be called in a ' +
'React function component or a custom React Hook function.';
context.report({node: hook, message});
Expand All @@ -626,7 +632,9 @@ const rule = {
// `use(...)` can be called in callbacks.
if (isSomewhereInsideComponentOrHook && !isUseIdentifier(hook)) {
const message =
`React Hook "${getSourceCode().getText(hook)}" cannot be called ` +
`React Hook "${getSourceCode().getText(
hook,
)}" cannot be called ` +
'inside a callback. React Hooks must be called in a ' +
'React function component or a custom React Hook function.';
context.report({node: hook, message});
Expand Down Expand Up @@ -681,18 +689,18 @@ const rule = {
Identifier(node) {
// This identifier resolves to a useEffectEvent function, but isn't being referenced in an
// effect or another event function. It isn't being called either.
if (
lastEffect == null &&
useEffectEventFunctions.has(node) &&
node.parent.type !== 'CallExpression'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The meaningful change here is that we error regardless of if the parent node is a call expression when we are not in an effect and the identifier is a uEE function. We tweak the error message based on whetehr it was called or not

) {
if (lastEffect == null && useEffectEventFunctions.has(node)) {
const message =
`\`${getSourceCode().getText(
node,
)}\` is a function created with React Hook "useEffectEvent", and can only be called from ` +
'the same component.' +
(node.parent.type === 'CallExpression'
? ''
: ' They cannot be assigned to variables or passed down.');
context.report({
node,
message:
`\`${getSourceCode().getText(
node,
)}\` is a function created with React Hook "useEffectEvent", and can only be called from ` +
'the same component. They cannot be assigned to variables or passed down.',
message,
});
}
},
Expand Down
Loading