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

Bug: eslint-plugin-react-hooks does not allow passing hook as props in jsx syntax #24108

Closed
alisajadih opened this issue Mar 16, 2022 · 8 comments
Labels
Resolution: Stale Automatically closed due to inactivity Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug

Comments

@alisajadih
Copy link

alisajadih commented Mar 16, 2022

React version: latest
demo:https://codesandbox.io/s/cranky-framework-p7r5vb?file=/src/App.tsx

The current behavior

jsx syntax does not work correctly.

 <>
    <IsolateRender useRender={() => useCustomeHook()} /> // error !
    <IsolateRender {...{ useRender: () => useCustomeHook() }} /> no error!
 </>

it shows this error:

React Hook "useCustomeHook" cannot be called inside a callback. React Hooks must be called in a React function component or a custom React Hook function. (react-hooks/rules-of-hooks)

The expected behavior

jsx syntax should work as object syntax.

@alisajadih alisajadih added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Mar 16, 2022
@vkurchatkin
Copy link

Why would you expect this to be allowed? It is explicitly stated that you can only use hooks in function components or custom hooks.

@mh-alahdadian
Copy link

Why would you expect this to be allowed? It is explicitly stated that you can only use hooks in function components or custom hooks.

Well this is not breaking rules of hooks , here we are passing hook to a component and If the second syntax is allowed, why isn't the first syntax allowed as well?

@HenriqueDerosa
Copy link

Hey @alisajadih, the documentation explains that we should use hooks only at the top level

Don’t call Hooks inside loops, conditions, or nested functions [...]

The example you sent is using the naming convention that identifies your function as a hook (with use prefix) but it's not an actual hook. It's pretty much like another react component as it only returns another JSX.

const useCustomeHook = () => {
  return <div />;
};

Possible solution

👉🏻 If you rename your useCustomHook function to be customHook it would not give you an error anymore.
In this case, it wouldn't give the error because it's not considered a hook.

Hooks' purpose

I guess this question came because the purpose of the hook is a bit confuse for you. I'd check the Introducing Hooks documentation, to make sure you can clarify this point. In summary, hooks are useful to deal with component lifecycle when it is just a simple function rather than a class with inheritance and predefined functions.

From the documentation:

They let you use state and other React features without writing a class [...]
Hooks allow you to reuse stateful logic without changing your component hierarchy [...]
Hooks let you split one component into smaller functions based on what pieces are related [...]

@alisajadih
Copy link
Author

Thanks for your answers. I know rules of hooks and hooks aspects. I want to make an inline custom hook (by passing anonymouse function as useRender prop). my problem is when I try try to pass anonymouse function like this:
<IsolateRender useRender={() => useCustomeHook()} />
It shows me an error. But when i try to pass it as object:
<IsolateRender {...{ useRender: () => useCustomeHook() }} />
It doesn't have any problem.
Also for clarifying the problem, this syntax works correctly as well:

const useCostomHook = () => {/*do something*/}
<IsolateRender useRender={useCustomeHook} /> 

@stephan-noel
Copy link

I think the misunderstanding here is 1) defining a custom hook and passing that as a prop which is then later invoked at the top level of a component and thus follows the rules of hooks vs 2) calling the hook when passing it as a prop.

The original code does 1), not 2) and so doesn't violate the rules of hooks.

I was just looking into this and it seems it's covered in the docs now:

https://react.dev/reference/rules/react-calls-components-and-hooks#dont-dynamically-use-hooks

Hooks should also not be dynamically used: for example, instead of doing dependency injection in a component by passing a hook as a value:

function ChatInput() {
  return <Button useData={useDataWithLogging} /> // 🔴 Bad: don't pass hooks as props
}

You should always inline the call of the hook into that component and handle any logic in there.

function ChatInput() {
  return <Button />
}

function Button() {
  const data = useDataWithLogging(); // ✅ Good: Use the hook directly
}

function useDataWithLogging() {
  // If there's any conditional logic to change the hook's behavior, it should be inlined into
  // the hook
}

This way, <Button /> is much easier to understand and debug. When Hooks are used in dynamic ways, it increases the complexity of your app greatly and inhibits local reasoning, making your team less productive in the long term. It also makes it easier to accidentally break the Rules of Hooks that hooks should not be called conditionally. If you find yourself needing to mock components for tests, it’s better to mock the server instead to respond with canned data. If possible, it’s also usually more effective to test your app with end-to-end tests.

In my opinion though, this section of the docs is too strong in its stance and doesn't offer any real alternative. Simply moving the hook down in many cases doesn't solve what you were probably trying to achieve.

I don't see the problem if you name the prop starting with "use" and pass it down because then I'd expect the eslint plugin to catch the improper usage of the hook prop so the following statement would be wrong:

It also makes it easier to accidentally break the Rules of Hooks that hooks should not be called conditionally.

I think the 2 main arguments are "local reasoning" which can depends on context, and the de-optimization it mentions React does. I'd say if the error would be kept because these are useful to know in general, then updating the error message to reflect the correct reason would be a good approach.

I could try to pick this up if the maintainers agree.

@stephan-noel
Copy link

stephan-noel commented Mar 23, 2024

Just did a bit of digging into the PR which added this to the docs, and it's more of a React Forget Compiler thing:

reactjs/react.dev#6680 (comment)

poteto 2 weeks ago
Tried to make it clearer by explaining it in the context of a non-compiled React app, but yeah this is more of a compiler specific rule. I suppose you could say it's more of a stylistic rule for non-compiled apps, but for React Compiler powered apps it becomes a hard rule.

I would prefer the error message to be more accurate still. Also "hard rule" would mean React will de-opt I suppose, not that you'd have a bug since it's probably just that the react compiler won't be able to automatically memoize.

Copy link

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

@github-actions github-actions bot added the Resolution: Stale Automatically closed due to inactivity label Jun 21, 2024
Copy link

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Stale Automatically closed due to inactivity Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug
Projects
None yet
Development

No branches or pull requests

5 participants