Bug: eslint-plugin-react-hooks rules-of-hooks misses hook usage in class properties with typescript-eslint v5+ parser

[ecraig12345]

React version: any version with hooks
eslint-plugin-react-hooks version: 4.6.0 / all (still repros in 5.0.0-canary-f81c0f1ed-20230927)

Steps To Reproduce

Link to code example: https://github.com/ecraig12345/repro-react-hooks-eslint

1. Install dependencies:
   • eslint-plugin-react-hooks (any version)
   • @typescript-eslint/parser v5 or greater
   • eslint v6 or greater
   • typescript (any version)
2. In the project's eslint config, set the parser to @typescript-eslint/parser and enable the react-hooks/rules-of-hooks rule

{
  parser: '@typescript-eslint/parser',
  plugins: ['react-hooks'],
  rules: {
    'react-hooks/rules-of-hooks': 'error',
  }
}

3. Add some code that calls a hook inside a class property function (not a class method)

import { useState } from 'react';
class Test {
  useHook = () => {
    useState();
  }
}

4. Run eslint on the project

The current behavior

No errors are flagged.

The expected behavior

One error is flagged.

The behavior was correct with @typescript-eslint/parser v4 (as demonstrated in the full repro).

Root cause

In @typescript-eslint/parser v5, the AST node type ClassProperty was renamed to PropertyDefinition. The rules-of-hooks rule needs to be updated to check for this additional type.

[eps1lon]

Thank you for raising this issue with such a detailed analysis.

I don't think the code you posted should be flagged. Since the class property is detected as a hook, you can call hooks inside it.

The code would only be problematic if new Test().useHook() would not be flagged if called outside of hooks/render.

So this seems to me like a bug that was fixed by switching to newer TypeScript ESLint parsers.

Could you add your snippet to the changes you proposed in #27435 to confirm it would still be allowed.

[hmhealey]

I've run into this in our code where the ESLint plugin misses that a hook is used within a property of a class component. Here's an example based on our code:

export class List extends React.PureComponent {
    renderRow = () => {
        const [value] = useState('value');

        return <li>{value}</li>;
    };

    render() {
        return (
            <ul>
                {this.renderRow()}
            </ul>
        );
    }
}

This isn't caught by ESLint because renderRow is defined as a property, but when this is rendered in the app, React errors out. If renderRow is changed to a regular method, ESLint catches the error correctly

[juhiz8]

Is My Facebook account on Sanger?! Couse i get these "React" emails and i dont know how to use github of c,c++ languages LähetettyOutlook for Android<https://aka.ms/AAb9ysg>

…

________________________________ From: Harrison Healey ***@***.***> Sent: Friday, March 15, 2024 7:37:15 PM To: facebook/react ***@***.***> Cc: Subscribed ***@***.***> Subject: Re: [facebook/react] Bug: eslint-plugin-react-hooks rules-of-hooks misses hook usage in class properties with typescript-eslint v5+ parser (Issue #27431) I've run into this in our code where the ESLint plugin misses that a hook is used within a property of a class component. Here's an example based on our code: export class List extends React.PureComponent { renderRow = () => { const [value] = useState('value'); return <li>{value}</li>; }; render() { return ( <ul> {this.renderRow()} </ul> ); } } This isn't caught by ESLint because renderRow is defined as a property, but when this is rendered in the app, React errors out. If renderRow is changed to a regular method, ESLint catches the error correctly — Reply to this email directly, view it on GitHub<#27431 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AZNMMUPUEWOQZZN5IBEB3U3YYMWUXAVCNFSM6AAAAAA5KOPDICVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMBQGEZDEMRXHA>. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[github-actions]

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!

[ecraig12345]

@eps1lon Is it ever valid for a hook to be defined as a class method/property? Either way, the problem is that right now it's inconsistent--hook usage in use___ class methods is flagged, but not in use___ class property functions (example):

[eps1lon]

Is it ever valid for a hook to be defined as a class method/property?

Sure. It's likely unnecessary since a class method implies the method is stateful which would likely not behave as expected at runtime. So any hook as a class method should be able to be rewritten as a normal function.

I guess we previously assume a hook in a class implies this class is a React component. We should probably continue with that logic.

[github-actions]

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!