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

rule to ensure forwardRef includes a ref argument. #3158

Closed
jquense opened this issue Dec 14, 2021 · 4 comments · Fixed by #3667
Closed

rule to ensure forwardRef includes a ref argument. #3158

jquense opened this issue Dec 14, 2021 · 4 comments · Fixed by #3667

Comments

@jquense
Copy link

jquense commented Dec 14, 2021

Howdy, not sure if this is a question or suggestion for a new rule, but i've not managed to find an existing rule to cover it.

But i'd like to find a rule that flags the following as an error, because the ref argument is not present.

import { forwardRef } from 'react';

const Button = forwardRef(props => <button {...props} />)

Whether or not the ref is used is already handled by no-unused-vars.

@ljharb
Copy link
Member

ljharb commented Dec 22, 2021

That seems like a useful thing to validate. Would the fix be "force use of a ref", or would it be "make it a functional component instead"? The latter seems to make sense to me - an autofix for that would be pretty aggressive in development though, altho a suggestion could work.

I am hesitant to add a whole new rule just for that one thing, but there's not really another place to put it. Thoughts?

@jquense
Copy link
Author

jquense commented Dec 23, 2021

Would the fix be "force use of a ref", or would it be "make it a functional component instead"? The latter seems to make sense to me - an autofix for that would be pretty aggressive in development though, altho a suggestion could work

IME this is less often a "I used forwardRef without intending to" vs "I intended to pass a ref through but forgot". I agree an autofix would be aggressive and probably frustrating in development.

I am hesitant to add a whole new rule just for that one thing

yeah its a small thing, I can't really think of a good alternative place though. Maybe it's not so bad as it's own rule though, a number of the existing rules are narrowly scoped.

@ljharb
Copy link
Member

ljharb commented Dec 30, 2021

alrighty, why not - let's add a forward-ref-uses-ref rule. It should not autofix, but it should offer two suggestions if possible:

  1. just a component, not a forwardRef
  2. add the ref argument and also attach it to the primary returned jsx element

@xkomiks

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants