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

Memo + ForwardRef = false positive? #2349

Closed
BjornThorsson opened this issue Jul 16, 2019 · 7 comments
Closed

Memo + ForwardRef = false positive? #2349

BjornThorsson opened this issue Jul 16, 2019 · 7 comments

Comments

@BjornThorsson
Copy link

BjornThorsson commented Jul 16, 2019

Eslint complains about myProp being missing in prop validation
when a forwardRef'd component is memoized.

const myComponent = memo(forwardRef( ({ myProp }, ref) => {
....
}

If I add myProp to prop validation.

const myComponent = ({ myProp }, ref) => {
....
}
myComponent.propTypes = { myProp: PropTypes.number.isRequired }
default export memo(forwardRef(myComponent));

Then eslint stops complaining, but now the site wont render:

Warning: forwardRef render functions do not support propTypes or defaultProps. Did you accidentally pass a React component?

Is there someway to use memo and forwardRef that eslint doesn't complain about?

@ljharb
Copy link
Member

ljharb commented Jul 16, 2019

forwardRef callbacks aren’t components and don’t take props or propTypes - if you have a forwardRef, why isn't it using the ref argument?

@BjornThorsson
Copy link
Author

forwardRef callbacks aren’t components and don’t take props or propTypes - if you have a forwardRef, why isn't it using the ref argument?

Whoops, you're right, forgot the 2nd param "Ref" for this example. Fixed.

I realized that forwardRef'd components don't have propTypes, what I'm saying is Eslint complains if I don't validate props using propTypes when I have a memoized forwardRef. If fowardRef doesn't have propTypes then a memoized forwardRef shouldnt either, yet eslint want's me to add propTypes to it.
Hence why i think this is a false positive.

@ljharb
Copy link
Member

ljharb commented Jul 16, 2019

If it only happens with the combination - ie, with memo and forwardRef, but not just with forwardRef - then it’s definitely a bug.

@BjornThorsson
Copy link
Author

If it only happens with the combination - ie, with memo and forwardRef, but not just with forwardRef - then it’s definitely a bug.

That's what i thought.

@yannickcr
Copy link
Member

Having multiple wrapper was not supported, it should be fixed in v7.14.3. Thanks for the report 🙂

Also you must add the propTypes to the final component:

const myComponent =  memo(forwardRef(({ myProp }, ref) => {
  // ...
}))
myComponent.propTypes = { myProp: PropTypes.number.isRequired }
default export myComponent;

@BjornThorsson
Copy link
Author

BjornThorsson commented Jul 22, 2019

Thanks for the report

Thanks for the fix!

@DaYesahh

This comment was marked as off-topic.

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

No branches or pull requests

4 participants