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: using hooks inside a named forwardRef callback causes a lint error #18254

Closed
hovsater opened this issue Mar 9, 2020 · 7 comments
Closed
Labels
Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug

Comments

@hovsater
Copy link

hovsater commented Mar 9, 2020

My understanding is that hooks are allowed within the render function passed to React.forwardRef. But the Eslint rule, react-hooks/rules-of-hooks, is displeased with the following code:

const Component = forwardRef(function renderComponent(props, ref) {
  useSomeHook();
  return <SomeComponent {...props} ref={ref} />;
});

However, removing the name of the callback function or capitalising the first letter, e.g., RenderComponent , causes the linter to stay happy.

I've also taken a look at #17220 where I can see a test that explicitly test for this case marking it as invalid. Why?

React version: 16.13.0

@hovsater hovsater added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Mar 9, 2020
@threepointone
Copy link
Contributor

This is to enforce a convention, and to be clear, that that function is a component definition. This means that it’ll show up in react dev tools with that name, etc. It’ll still work if you don’t capitalise it, but the intent is clearer when it’s visually identifiable as a component. That’s why we enforce it as a lint rule. Hope this clarifies things!

@hovsater
Copy link
Author

@threepointone thanks for the quick reply! Appreciate it! 🙂

According to previous discussions, the inner function (the callback) isn't a component definition but a render function. @gaearon mention it here for instance.

As I understand it the returned value from forwardRef is a component though but perhaps it doesn't make a difference in this case?

@eps1lon
Copy link
Collaborator

eps1lon commented Mar 10, 2020

Would it help if the linter would explain that function components and forwardRef render functions need to have a capitalized name?

I think you shouldn't call it just a "render function" because that might imply (for historic reasons) that you can call it yourself (e.g. inside a loop) which would break if you'd use hooks inside it. But a component might also be misleading because you can't pass it to createElement.

@hovsater
Copy link
Author

hovsater commented Mar 10, 2020

@eps1lon I think that would be a step in the right direction. Right now, it's confusing since function () {}, function renderComponent() {} and function RenderComponent() {} are legal but the linter only allow for the first and last one.

So if the linter would say "The render function to forwardRef must be capitalised or anonymous" instead of saying that hooks are illegal it's an improvement.

@eps1lon
Copy link
Collaborator

eps1lon commented Mar 10, 2020

Up to the core team in the end but I think I'd hope they welcome a PR that refines the error message if this is an immediate argument of React.forwardRef. This should cover most cases.

@hovsater
Copy link
Author

hovsater commented Mar 10, 2020

Yeah, I'd be happy to help out if it's of interest. 🙁

@threepointone
Copy link
Contributor

Oh hmm yeah. Lemme think about this some more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug
Projects
None yet
Development

No branches or pull requests

3 participants