Update RulesOfHooks with useEvent rules

[poteto]

Summary

This update to the RulesOfHooks lint rule checks that functions created with useEvent must be invoked locally within the component its defined in at least once. They can't be passed down directly as a reference to child components.

In the ExhaustiveDeps lint rule, useEvent is treated as stable, and thus does not need to be included within dependency lists.

Supersedes #25121.

How did you test this change?

Added new tests.

[sizebot]

Comparing: 112d049...54765ee

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js	=	135.11 kB	135.11 kB	=	43.35 kB	43.35 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	142.57 kB	142.57 kB	=	45.54 kB	45.54 kB
facebook-www/ReactDOM-prod.classic.js	=	490.10 kB	490.10 kB	=	87.23 kB	87.23 kB
facebook-www/ReactDOM-prod.modern.js	=	475.39 kB	475.39 kB	=	85.04 kB	85.04 kB
facebook-www/ReactDOMForked-prod.classic.js	=	490.10 kB	490.10 kB	=	87.23 kB	87.23 kB
oss-experimental/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.production.min.js	+5.96%	25.66 kB	27.19 kB	+4.56%	8.79 kB	9.19 kB
oss-experimental/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js	+5.74%	87.53 kB	92.55 kB	+5.25%	20.77 kB	21.86 kB
oss-stable-semver/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js	+5.60%	87.53 kB	92.43 kB	+5.16%	20.77 kB	21.84 kB
oss-stable/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js	+5.60%	87.53 kB	92.43 kB	+5.16%	20.77 kB	21.84 kB
oss-stable-semver/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.production.min.js	+4.80%	25.66 kB	26.89 kB	+3.70%	8.79 kB	9.11 kB
oss-stable/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.production.min.js	+4.80%	25.66 kB	26.89 kB	+3.70%	8.79 kB	9.11 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-experimental/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.production.min.js	+5.96%	25.66 kB	27.19 kB	+4.56%	8.79 kB	9.19 kB
oss-experimental/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js	+5.74%	87.53 kB	92.55 kB	+5.25%	20.77 kB	21.86 kB
oss-stable-semver/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js	+5.60%	87.53 kB	92.43 kB	+5.16%	20.77 kB	21.84 kB
oss-stable/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js	+5.60%	87.53 kB	92.43 kB	+5.16%	20.77 kB	21.84 kB
oss-stable-semver/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.production.min.js	+4.80%	25.66 kB	26.89 kB	+3.70%	8.79 kB	9.11 kB
oss-stable/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.production.min.js	+4.80%	25.66 kB	26.89 kB	+3.70%	8.79 kB	9.11 kB

Generated by 🚫 dangerJS against 54765ee

[gaearon]

I'm not sure this approach quite makes sense to me. Maybe we need to discuss the rules more.

It seems like the approach you're taking is to restrict some calls to useEvent functions and passing it as JSX arguments.

I imagined the rules a bit different. I think what we need to forbid is referencing without a call. This would automatically forbid passing in JSX. But it would also forbid any other things like

const foo = onClick
return <Bar onClick={foo} />

and other variations. I think the key insight is that a bare reference like onClick is bad, but a call like onClick() is good.

And then, we can add back a special case that allows you to have a bare reference directly inside useEffect as a convenience.

useEffect(() => {
  let id = setInterval(onTick, 100)
  return () => clearInterval(onTick)
}, [])

but this is kind of syntax sugar and not essential to the rule.

It's forbidding non-call references that's essential.

[poteto]

@gaearon yeah, "inverting" the logic is a great point – will fix!

[poteto]

@gaearon So the lint rule now is more resilient to aliasing (and we no longer need to mess with scopes – yay!), but one small tradeoff is that you immediately get a red squiggly when you first define a function with useEvent, since it's not called anywhere yet. That's probably alright, since a similar issue happens in the ExhaustiveDeps checks.

[Huxpro]

Shall we add test cases for referencing event functions from useEffect and useEvent?

Also I'm not sure what to do with this

const onClick = useEvent(() => {});
let foo;
useEffect(() => {
  foo = onClick // allowed since it's within the effect scope
})
return <Bar onClick={foo} /> // we don't know foo

[Huxpro]

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

[poteto]

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?

[poteto]

@Huxpro @gaearon Updated to only resolve useEvent violations if the identifier is an argument into a CallExpr. But now I'm not so sure if we should still have the useEffect shorthand – we're relying on the assumption that something later invokes the useEvent function but that's not always the case. setInterval(onClick, 100) for example is a case we know that the browser will call the function, but onClick = myFunction(onClick) could still be passed by reference. Maybe it's better to just drop the shorthand altogether? Or allowlist some browser APIs like setTimeout and co.

[gaearon]

we're relying on the assumption that something later invokes the useEvent function but that's not always the case. setInterval(onClick, 100) for example is a case we know that the browser will call the function, but onClick = myFunction(onClick) could still be passed by reference

Why is it a problem for it to be passed in this case?

I think in the normal rendering path, passing it is a problem becauase it could be used as a reactive dependency during rendering below but the reactivity is "erased". However, once you're in an Effect, you can't feed that function into a rendering path (I guess unless you put it into state? this seems pretty unlikely).

Note that arbitrary functions are different because they might be called during render. So it's not OK e.g. renderItem could return <Foo onClick={onClick} /> and that gets fed into rendering. But Effects are always called after render is already complete.

[gaearon]

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?

[poteto]

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?

[gaearon]

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

[gaearon]

looks good but see last comment

[poteto]

This last test is passing but it looks like I'm getting rate limited from GitHub so the status can't update