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

feature request: [react/prop-types] recognize generics passed to React.memo #3866

Open
2 tasks done
NuclleaR opened this issue Dec 12, 2024 · 7 comments
Open
2 tasks done

Comments

@NuclleaR
Copy link

Is there an existing issue for this?

  • I have searched the existing issues and my issue is unique
  • My issue appears in the command-line and not only in the text editor

Description Overview

eslint v8.57.1
eslint-plugin-react: 7.37.2

Image

Image

Expected Behavior

eslint-plugin-react version

v7.37.2

eslint version

v8.57.1

node version

v18.17.1

@NuclleaR NuclleaR added the bug label Dec 12, 2024
@rjgotten
Copy link

rjgotten commented Mar 4, 2025

@NuclleaR
FWIW - this problem occurs because you're using

memo<Props>(({ foo, bar }) => {
  // ...
});

If you instead use

memo(({ foo, bar }: Props) => {
  // ...
});

the prop-types lint rule works as expected. Whoever wrote it probably is not looking up the assigned property type in the correct manner and is only looking for a directly assigned type.

@NuclleaR
Copy link
Author

NuclleaR commented Mar 4, 2025

@rjgotten this is not how it should work. If React allows us to use generics, eslint should respect them

@ljharb
Copy link
Member

ljharb commented Mar 4, 2025

@NuclleaR sure but someone would have to manually update the rule to support the generics, which are a recent addition to the TS types. Are you volunteering?

@ljharb ljharb changed the title [Bug]: [react/prop-types] False positive when wrapped with React.memo feature request: [react/prop-types] recognize generics passed to React.memo Mar 4, 2025
@rjgotten
Copy link

rjgotten commented Mar 5, 2025

@NuclleaR
@rjgotten this is not how it should work. If React allows us to use generics, eslint should respect them

Oh, different from some of the maintainers - apparently, I actually agree with you there.
It's a bug. Clear as day.
It means the lint rule is poorly written and is taking shortcuts to establish the parameter's concrete type, that it shouldn't be. It was always written wrong. It just so happened to not fail while React.memo didn't have a generic parameter.

I was just giving you a potential workaround.

@ljharb
Copy link
Member

ljharb commented Mar 5, 2025

@rjgotten no, it means the lint rule was written before the TS type took generics, and nobody noticed to update it until this issue.

@rjgotten
Copy link

rjgotten commented Mar 6, 2025

@ljharb
@rjgotten no, it means the lint rule was written before the TS type took generics, and nobody noticed to update it until this issue.

No, it means the rule was implemented poorly.
If it was implemented correctly, it would've invoked the TypeScript type checker API, which you simply pass the relevant node (the props parameter) and the type checker would spit out the node's correct type according to the type checker. No matter whether the type would be set explicitly or would come from setting the generic constraint.

See also:
https://typescript-eslint.io/developers/custom-rules#typed-rules

Concrete relevant lines being:

const services = ESLintUtils.getParserServices(context);
const type = services.getTypeAtLocation(node);

If the rule were implemented correctly, it would have been implemented as two rules.
A non-typed base eslint rule for JavaScript projects.
And a typed rule relying on typescript-eslint and its integration facilities with the TypeScript type checker for TypeScript projects.

@ljharb
Copy link
Member

ljharb commented Mar 6, 2025

@rjgotten PRs are welcome, but that is not the correct description of how a rule should be implemented, because this is a JavaScript project. It is not a bug, it’s just a missing enhancement.

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

No branches or pull requests

3 participants