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

Update RulesOfHooks with useEvent rules #25285

Merged
merged 6 commits into from
Sep 23, 2022
Merged
Show file tree
Hide file tree
Changes from 2 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 @@ -1452,6 +1452,18 @@ const tests = {
}
`,
},
{
poteto marked this conversation as resolved.
Show resolved Hide resolved
code: normalizeIndent`
function MyComponent({ theme }) {
const onStuff = useEvent(() => {
showNotification(theme);
});
useEffect(() => {
onStuff();
}, []);
}
`,
},
],
invalid: [
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -406,6 +406,82 @@ const tests = {
const [myState, setMyState] = useState(null);
}
`,
`
// Valid because functions created with useEvent can be called in a useEffect.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: shall we just call this "event functions" like the doc (https://beta.reactjs.org/learn/separating-events-from-effects)? It's conciser and feels first classy

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure what exactly we want to call it (after all the recent bikeshedding) since useEvent might be renamed, hence why I've been avoiding giving it a name. Maybe @acdlite can weigh in on what to call it in the meantime?

function MyComponent({ theme }) {
const onClick = useEvent(() => {
showNotification(theme);
});
useEffect(() => {
onClick();
});
}
`,
`
// Valid because functions created with useEvent can be called in closures.
function MyComponent({ theme }) {
const onClick = useEvent(() => {
showNotification(theme);
});
return <Child onClick={() => onClick()}></Child>;
}
`,
`
// Valid because functions created with useEvent can be called in closures.
function MyComponent({ theme }) {
const onClick = useEvent(() => {
showNotification(theme);
});
const onClick2 = () => { onClick() };
poteto marked this conversation as resolved.
Show resolved Hide resolved
const onClick3 = useCallback(() => onClick(), []);
return <>
<Child onClick={onClick2}></Child>
<Child onClick={onClick3}></Child>
</>;
}
`,
`
// Valid because functions created with useEvent can be passed by reference in useEffect.
function MyComponent({ theme }) {
const onClick = useEvent(() => {
showNotification(theme);
});
useEffect(() => {
let id = setInterval(onClick, 100);
return () => clearInterval(onClick);
}, []);
}
`,
`
const MyComponent = ({theme}) => {
const onClick = useEvent(() => {
showNotification(theme);
});
return <Child onClick={() => onClick()}></Child>;
};
`,
`
function MyComponent({ theme }) {
const notificationService = useNotifications();
const showNotification = useEvent((text) => {
notificationService.notify(theme, text);
});
const onClick = useEvent((text) => {
showNotification(text);
});
return <Child onClick={(text) => onClick(text)} />
}
`,
`
function MyComponent({ theme }) {
useEffect(() => {
onClick();
});
const onClick = useEvent(() => {
showNotification(theme);
});
}
`,
],
invalid: [
{
Expand Down Expand Up @@ -449,7 +525,7 @@ const tests = {
},
{
code: `
// This is a false positive (it's valid) that unfortunately
// This is a false positive (it's valid) that unfortunately
// we cannot avoid. Prefer to rename it to not start with "use"
class Foo extends Component {
render() {
Expand Down Expand Up @@ -971,6 +1047,64 @@ const tests = {
`,
errors: [classError('useState')],
},
{
code: `
function MyComponent({ theme }) {
const onClick = useEvent(() => {
showNotification(theme);
});
return <Child onClick={onClick}></Child>;
}
`,
errors: [useEventError('onClick')],
},
{
code: `
// This should error even though it shares an identifier name with the below
function MyComponent({theme}) {
const onClick = useEvent(() => {
showNotification(theme)
});
return <Child onClick={onClick} />
}

// The useEvent function shares an identifier name with the above
function MyOtherComponent({theme}) {
const onClick = useEvent(() => {
showNotification(theme)
});
return <Child onClick={() => onClick()} />
}
`,
errors: [{...useEventError('onClick'), line: 4}],
},
{
code: `
const MyComponent = ({ theme }) => {
const onClick = useEvent(() => {
showNotification(theme);
});
return <Child onClick={onClick}></Child>;
}
poteto marked this conversation as resolved.
Show resolved Hide resolved
`,
errors: [useEventError('onClick')],
},
{
code: `
// Invalid because onClick is being aliased to foo but not invoked
function MyComponent({ theme }) {
const onClick = useEvent(() => {
showNotification(theme);
});
let foo;
useEffect(() => {
foo = onClick;
});
return <Bar onClick={foo} />
}
`,
errors: [useEventError('onClick')],
},
],
};

Expand Down Expand Up @@ -1031,6 +1165,14 @@ function classError(hook) {
};
}

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

// For easier local testing
if (!process.env.CI) {
let only = [];
Expand Down
8 changes: 8 additions & 0 deletions packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,8 @@ export default {
// ^^^ true for this reference
// const ref = useRef()
// ^^^ true for this reference
// const onStuff = useEvent(() => {})
// ^^^ true for this reference
// False for everything else.
function isStableKnownHookValue(resolved) {
if (!isArray(resolved.defs)) {
Expand Down Expand Up @@ -223,6 +225,12 @@ export default {
if (name === 'useRef' && id.type === 'Identifier') {
// useRef() return value is stable.
return true;
} else if (
(name === 'experimental_useEvent' || name === 'useEvent') &&
gaearon marked this conversation as resolved.
Show resolved Hide resolved
id.type === 'Identifier'
) {
// useEvent() return value is stable.
gaearon marked this conversation as resolved.
Show resolved Hide resolved
return true;
} else if (name === 'useState' || name === 'useReducer') {
// Only consider second value in initializing tuple stable.
if (
Expand Down
100 changes: 100 additions & 0 deletions packages/eslint-plugin-react-hooks/src/RulesOfHooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,13 @@ function isInsideComponentOrHook(node) {
return false;
}

function isUseEventIdentifier(node) {
return (
node.type === 'Identifier' &&
(node.name === 'useEvent' || node.name === 'experimental_useEvent')
gaearon marked this conversation as resolved.
Show resolved Hide resolved
);
}

export default {
meta: {
type: 'problem',
Expand All @@ -110,8 +117,45 @@ export default {
},
},
create(context) {
let lastEffect = null;
const codePathReactHooksMapStack = [];
const codePathSegmentStack = [];
const useEventViolations = new Set();

// For a given AST node, iterate through the top level statements and add all useEvent
// definitions. We can do this in non-Program nodes because we can rely on the assumption that
// useEvent functions can only be declared within a component or hook at its top level.
function addAllUseEventViolations(node) {
if (node.body.type !== 'BlockStatement') return;
for (const statement of node.body.body) {
if (statement.type !== 'VariableDeclaration') continue;
for (const declaration of statement.declarations) {
if (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we think of any case where we'd need to go deeper or recurse? I think the only valid one I can come up with is { } blocks that aren't conditional. Is there more?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think one generalized case where we might need to go deeper is if the event function is reassigned to a variable instead of being initialized in a variable declaration:

function Component() {
  let onClick;
  {
    onClick = useEvent(...);
  }
  (function weird() {
    onClick = useEvent(...);
  })();
  // etc
}

But I don't know if this is something we want to support? I may have misunderstood the overall goal of the lint rules: do we aim to be correct in all cases, or only in the "idiomatic" case with some small exceptions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yea this is not worth supporting. I think only idiomatic usage is relevant, and I guess people don't use raw blocks much either.

declaration.type === 'VariableDeclarator' &&
declaration.init &&
declaration.init.type === 'CallExpression' &&
declaration.init.callee &&
isUseEventIdentifier(declaration.init.callee)
) {
useEventViolations.add(declaration.id);
}
}
}
}

// Resolve a useEvent violation, ie the useEvent created function was called.
function resolveUseEventViolation(scope, ident) {
if (scope.references == null || useEventViolations.size === 0) return;
for (const ref of scope.references) {
if (ref.resolved == null) continue;
const [useEventFunctionIdentifier] = ref.resolved.identifiers;
if (ident.name === useEventFunctionIdentifier.name) {
useEventViolations.delete(useEventFunctionIdentifier);
break;
}
}
}

return {
// Maintain code segment path stack as we traverse.
onCodePathSegmentStart: segment => codePathSegmentStack.push(segment),
Expand Down Expand Up @@ -522,6 +566,62 @@ export default {
}
reactHooks.push(node.callee);
}

const scope = context.getScope();
// useEvent: Resolve a function created with useEvent that is invoked locally at least once.
// OK - onClick();
resolveUseEventViolation(scope, node.callee);

// useEvent: useEvent functions can be passed by reference within useEffect
if (
node.callee.type === 'Identifier' &&
node.callee.name === 'useEffect' &&
node.arguments.length > 0
) {
// Denote that we have traversed into a useEffect call, and stash the CallExpr for
// comparison later when we exit
lastEffect = node;
}
},

Identifier(node) {
// OK - useEffect(() => { setInterval(onClick, ...) }, []);
if (lastEffect != null && node.parent.type === 'CallExpression') {
resolveUseEventViolation(context.getScope(), node);
}
},

'CallExpression:exit'(node) {
if (node === lastEffect) {
lastEffect = null;
}
},

FunctionDeclaration(node) {
// function MyComponent() { const onClick = useEvent(...) }
if (isInsideComponentOrHook(node)) {
addAllUseEventViolations(node);
}
},

ArrowFunctionExpression(node) {
// const MyComponent = () => { const onClick = useEvent(...) }
if (isInsideComponentOrHook(node)) {
addAllUseEventViolations(node);
}
},

'Program:exit'(_node) {
for (const node of useEventViolations.values()) {
context.report({
node,
message:
`\`${context.getSource(
node,
)}\` is a function created with React Hook "useEvent", and can only be called from ` +
'the same component. They cannot be assigned to variables or passed down.',
});
}
},
};
},
Expand Down