react icon indicating copy to clipboard operation
react copied to clipboard

[DevTools Bug]: Inconsistent enforcement of using hooks in callbacks

Open eisenpony opened this issue 4 years ago • 5 comments

Website or app

https://codesandbox.io/s/distracted-chihiro-u1mc2

Repro steps

The rules-for-hooks linter seems to be applying rules about using hooks in callbacks inconsistently.

Using a hook in this callback is okay const useNamed = async () => useQuery(); And subsequently sending it as a prop is also okay <Foo useData={useNamed} />

However, assigning the anonymous callback directly in the prop triggers the linter rule "React hook cannot be called inside a callback" <Foo useData={async () => useQuery()} />

I'm having trouble understanding the difference. They appear to be equivalent code but with different results from the linter. In which case is the linter correct?

Sandbox here: https://codesandbox.io/s/distracted-chihiro-u1mc2

How often does this bug happen?

Every time

DevTools package (automated)

No response

DevTools version (automated)

No response

Error message (automated)

No response

Error call stack (automated)

No response

Error component stack (automated)

No response

GitHub query string (automated)

No response

eisenpony avatar Feb 03 '22 03:02 eisenpony

Hey! Thanks for reporting! The reason for the rules-for-hooks linter is because React relies on the assumption that you call the same hooks every render. So both of these cases would break this rule. If you're curious about this check out this in depth explanation for why this is the case here.

So here, the linter would be incorrect for the useNamed case. In general, there are definitely ways to get around the linter. However, if you understand the reason for the linter itself you'll be able to avoid bugs that result as of it.

Right now, no one officially owns the lint rules. However, if you'd like to submit a fix for this we would be grateful!

lunaruan avatar Feb 03 '22 16:02 lunaruan

Thanks Luna!

So I think you're saying the linter should flag the first case

const useNamed = async () => useQuery();
<Foo useData={useNamed} />

Except, I'm a bit confused about that.

Since the hook is passed into a prop named use*, isn't it safe to allow both cases, provided the prop follows all rules for hooks in the component?

eisenpony avatar Feb 03 '22 20:02 eisenpony

Why would need to pass the hook as a prop in the first place? Technically, as long as it follows the rules of hooks it wouldn't break anything, but you can also just import the hook directly and use it in the component you actually need it in, which is the way we recommend.

lunaruan avatar Feb 03 '22 20:02 lunaruan

This is a type of dependency injection, so I think passing hooks as props to components opens up architectural choices, making React less opinionated.

One reason you might want to do this is to decouple components from framework choices. So a component could be used by an app that settles on tannerlinsley/react-query for data binding, and another app that settles with mobxjs/mobx. Only the parent component knows about this choice and injects the correct adapters to its children.

Another reason is to inline logic in your render function with an anonymous function. Sometimes this can improve readability and sometimes it can eliminate a lot of boilerplate by giving us access to local context via a closure. Of course, the rules would need to apply to hooks used inside the anonymous function and the function, which effectively becomes a custom hook, should only ever be assigned to a useX prop.

I know there are alternatives to solve the problems I gave above, but dependency injection is often my preferred tool for the job. The linter seems to be handcuffing me here instead of helping.

The final argument I can make is that callbacks of all types were supported as props in class based components, so this feels like a bit of a regression.

eisenpony avatar Feb 03 '22 21:02 eisenpony

Hello! I would like to add my 2 cents on this discussion. Because, I'm trying to use dependency injection with React. I am quite a beginner on React but the closest I've found to doing dependency injection with React is using Hooks (useContext) and the Context API.

I think props are not suitable for dependency injection because you have to pass them through all parent components of the component where you want to use an external dependency.

nassimerrahoui avatar Oct 27 '22 16:10 nassimerrahoui