Skip to content

Commit 97cdd5d

Browse files
jbrown215jbrown215
authored
[eslint] Do not allow useEffectEvent fns to be called in arbitrary closures (facebook#33544)
Summary: useEffectEvent is meant to be used specifically in combination with useEffect, and using the feature in arbitrary closures can lead to surprising reactivity semantics. In order to minimize risk in the experimental rollout, we are going to restrict its usage to being called directly inside an effect or another useEffectEvent, effectively enforcing the function coloring statically. Without an effect system this is the best we can do.
1 parent eb7f8b4 commit 97cdd5d

File tree

2 files changed

+78
-70
lines changed

2 files changed

+78
-70
lines changed

‎packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js‎

Lines changed: 56 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -1343,33 +1343,6 @@ if (__EXPERIMENTAL__) {
13431343
}
13441344
`,
13451345
},
1346-
{
1347-
code: normalizeIndent`
1348-
// Valid because functions created with useEffectEvent can be called in closures.
1349-
function MyComponent({ theme }) {
1350-
const onClick = useEffectEvent(() => {
1351-
showNotification(theme);
1352-
});
1353-
return <Child onClick={() => onClick()}></Child>;
1354-
}
1355-
`,
1356-
},
1357-
{
1358-
code: normalizeIndent`
1359-
// Valid because functions created with useEffectEvent can be called in closures.
1360-
function MyComponent({ theme }) {
1361-
const onClick = useEffectEvent(() => {
1362-
showNotification(theme);
1363-
});
1364-
const onClick2 = () => { onClick() };
1365-
const onClick3 = useCallback(() => onClick(), []);
1366-
return <>
1367-
<Child onClick={onClick2}></Child>
1368-
<Child onClick={onClick3}></Child>
1369-
</>;
1370-
}
1371-
`,
1372-
},
13731346
{
13741347
code: normalizeIndent`
13751348
// Valid because functions created with useEffectEvent can be passed by reference in useEffect
@@ -1380,47 +1353,39 @@ if (__EXPERIMENTAL__) {
13801353
});
13811354
const onClick2 = useEffectEvent(() => {
13821355
debounce(onClick);
1356+
debounce(() => onClick());
1357+
debounce(() => { onClick() });
1358+
deboucne(() => debounce(onClick));
13831359
});
13841360
useEffect(() => {
1385-
let id = setInterval(onClick, 100);
1361+
let id = setInterval(() => onClick(), 100);
13861362
return () => clearInterval(onClick);
13871363
}, []);
1388-
return <Child onClick={() => onClick2()} />
1364+
return null;
13891365
}
13901366
`,
13911367
},
1392-
{
1393-
code: normalizeIndent`
1394-
const MyComponent = ({theme}) => {
1395-
const onClick = useEffectEvent(() => {
1396-
showNotification(theme);
1397-
});
1398-
return <Child onClick={() => onClick()}></Child>;
1399-
};
1400-
`,
1401-
},
14021368
{
14031369
code: normalizeIndent`
14041370
function MyComponent({ theme }) {
1405-
const notificationService = useNotifications();
1406-
const showNotification = useEffectEvent((text) => {
1407-
notificationService.notify(theme, text);
1371+
useEffect(() => {
1372+
onClick();
14081373
});
1409-
const onClick = useEffectEvent((text) => {
1410-
showNotification(text);
1374+
const onClick = useEffectEvent(() => {
1375+
showNotification(theme);
14111376
});
1412-
return <Child onClick={(text) => onClick(text)} />
14131377
}
14141378
`,
14151379
},
14161380
{
14171381
code: normalizeIndent`
14181382
function MyComponent({ theme }) {
1419-
useEffect(() => {
1420-
onClick();
1383+
const onEvent = useEffectEvent((text) => {
1384+
console.log(text);
14211385
});
1422-
const onClick = useEffectEvent(() => {
1423-
showNotification(theme);
1386+
1387+
useEffect(() => {
1388+
onEvent('Hello world');
14241389
});
14251390
}
14261391
`,
@@ -1437,7 +1402,7 @@ if (__EXPERIMENTAL__) {
14371402
return <Child onClick={onClick}></Child>;
14381403
}
14391404
`,
1440-
errors: [useEffectEventError('onClick')],
1405+
errors: [useEffectEventError('onClick', false)],
14411406
},
14421407
{
14431408
code: normalizeIndent`
@@ -1456,8 +1421,23 @@ if (__EXPERIMENTAL__) {
14561421
});
14571422
return <Child onClick={() => onClick()} />
14581423
}
1424+
1425+
// The useEffectEvent function shares an identifier name with the above
1426+
function MyLastComponent({theme}) {
1427+
const onClick = useEffectEvent(() => {
1428+
showNotification(theme)
1429+
});
1430+
useEffect(() => {
1431+
onClick(); // No error here, errors on all other uses
1432+
onClick;
1433+
})
1434+
return <Child />
1435+
}
14591436
`,
1460-
errors: [{...useEffectEventError('onClick'), line: 7}],
1437+
errors: [
1438+
{...useEffectEventError('onClick', false), line: 7},
1439+
{...useEffectEventError('onClick', true), line: 15},
1440+
],
14611441
},
14621442
{
14631443
code: normalizeIndent`
@@ -1468,7 +1448,7 @@ if (__EXPERIMENTAL__) {
14681448
return <Child onClick={onClick}></Child>;
14691449
}
14701450
`,
1471-
errors: [useEffectEventError('onClick')],
1451+
errors: [useEffectEventError('onClick', false)],
14721452
},
14731453
{
14741454
code: normalizeIndent`
@@ -1481,7 +1461,7 @@ if (__EXPERIMENTAL__) {
14811461
return <Bar onClick={foo} />
14821462
}
14831463
`,
1484-
errors: [{...useEffectEventError('onClick'), line: 7}],
1464+
errors: [{...useEffectEventError('onClick', false), line: 7}],
14851465
},
14861466
{
14871467
code: normalizeIndent`
@@ -1497,7 +1477,27 @@ if (__EXPERIMENTAL__) {
14971477
return <Child onClick={onClick} />
14981478
}
14991479
`,
1500-
errors: [useEffectEventError('onClick')],
1480+
errors: [useEffectEventError('onClick', false)],
1481+
},
1482+
{
1483+
code: normalizeIndent`
1484+
// Invalid because functions created with useEffectEvent cannot be called in arbitrary closures.
1485+
function MyComponent({ theme }) {
1486+
const onClick = useEffectEvent(() => {
1487+
showNotification(theme);
1488+
});
1489+
const onClick2 = () => { onClick() };
1490+
const onClick3 = useCallback(() => onClick(), []);
1491+
return <>
1492+
<Child onClick={onClick2}></Child>
1493+
<Child onClick={onClick3}></Child>
1494+
</>;
1495+
}
1496+
`,
1497+
errors: [
1498+
useEffectEventError('onClick', true),
1499+
useEffectEventError('onClick', true),
1500+
],
15011501
},
15021502
];
15031503
}
@@ -1559,11 +1559,11 @@ function classError(hook) {
15591559
};
15601560
}
15611561

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

‎packages/eslint-plugin-react-hooks/src/rules/RulesOfHooks.ts‎

Lines changed: 22 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -541,7 +541,9 @@ const rule = {
541541
context.report({
542542
node: hook,
543543
message:
544-
`React Hook "${getSourceCode().getText(hook)}" may be executed ` +
544+
`React Hook "${getSourceCode().getText(
545+
hook,
546+
)}" may be executed ` +
545547
'more than once. Possibly because it is called in a loop. ' +
546548
'React Hooks must be called in the exact same order in ' +
547549
'every component render.',
@@ -596,7 +598,9 @@ const rule = {
596598
) {
597599
// Custom message for hooks inside a class
598600
const message =
599-
`React Hook "${getSourceCode().getText(hook)}" cannot be called ` +
601+
`React Hook "${getSourceCode().getText(
602+
hook,
603+
)}" cannot be called ` +
600604
'in a class component. React Hooks must be called in a ' +
601605
'React function component or a custom React Hook function.';
602606
context.report({node: hook, message});
@@ -613,7 +617,9 @@ const rule = {
613617
} else if (codePathNode.type === 'Program') {
614618
// These are dangerous if you have inline requires enabled.
615619
const message =
616-
`React Hook "${getSourceCode().getText(hook)}" cannot be called ` +
620+
`React Hook "${getSourceCode().getText(
621+
hook,
622+
)}" cannot be called ` +
617623
'at the top level. React Hooks must be called in a ' +
618624
'React function component or a custom React Hook function.';
619625
context.report({node: hook, message});
@@ -626,7 +632,9 @@ const rule = {
626632
// `use(...)` can be called in callbacks.
627633
if (isSomewhereInsideComponentOrHook && !isUseIdentifier(hook)) {
628634
const message =
629-
`React Hook "${getSourceCode().getText(hook)}" cannot be called ` +
635+
`React Hook "${getSourceCode().getText(
636+
hook,
637+
)}" cannot be called ` +
630638
'inside a callback. React Hooks must be called in a ' +
631639
'React function component or a custom React Hook function.';
632640
context.report({node: hook, message});
@@ -681,18 +689,18 @@ const rule = {
681689
Identifier(node) {
682690
// This identifier resolves to a useEffectEvent function, but isn't being referenced in an
683691
// effect or another event function. It isn't being called either.
684-
if (
685-
lastEffect == null &&
686-
useEffectEventFunctions.has(node) &&
687-
node.parent.type !== 'CallExpression'
688-
) {
692+
if (lastEffect == null && useEffectEventFunctions.has(node)) {
693+
const message =
694+
`\`${getSourceCode().getText(
695+
node,
696+
)}\` is a function created with React Hook "useEffectEvent", and can only be called from ` +
697+
'the same component.' +
698+
(node.parent.type === 'CallExpression'
699+
? ''
700+
: ' They cannot be assigned to variables or passed down.');
689701
context.report({
690702
node,
691-
message:
692-
`\`${getSourceCode().getText(
693-
node,
694-
)}\` is a function created with React Hook "useEffectEvent", and can only be called from ` +
695-
'the same component. They cannot be assigned to variables or passed down.',
703+
message,
696704
});
697705
}
698706
},

0 commit comments

Comments
 (0)