[eslint] Check forwardRef callbacks #17255
Conversation
dprgarner
changed the title
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 54a180a:
|
Details of bundled changes.Comparing: f4148b2...54a180a eslint-plugin-react-hooks
Size changes (stable) |
Details of bundled changes.Comparing: f4148b2...54a180a eslint-plugin-react-hooks
Size changes (experimental) |
There was a problem hiding this comment.
The reason these tests fail without changing the implementation is that you didn't name the function not because it's passed to forwardRef. React.forWardRef(function Foo() {}) will be linted for rules of hooks.
It also encourages you to name the function or variable passed to forwardRef so that react can compute a proper display name (besides "unknown").
But this should probably be covered by a different rule.
The only issue I would have with the tests is that you didn't specify props nor ref argument which doesn't make much sense at the moment.
And it might make sense to apply this to React.memo as well.
|
Thanks for the feedback. I'll add explicit props and ref arguments to the test cases, and generalise this to check The react/display-name rule checks that components have a display name. In the codebase where I spotted this issue, we set Should there be a linting rule enforcing CapitalCase function names in the arguments of forwardRef, though? Is the argument actually a component? The function signature isn't the same as for a functional component, and the forwardRef docs refer to the argument as a "render function" and don't use CapitalCase when naming it. Does this mean that the function argument to |
| }); | ||
| `, | ||
| errors: [conditionalError('useCustomHook')], | ||
| }, |
There was a problem hiding this comment.
Can we also add tests verifying how we handle React.whatever(props => ...) or whatever(props => ...)? i.e. the case where isReactFunction returns false.
There was a problem hiding this comment.
I've added some more tests. The existing behaviour is to skip checking unnamed callbacks - there's a regression test in place for this.
Closes #17220
This checks that the rules of hooks apply to callback arguments of
forwardRef.I'm unsure whether this counts as a backwards-incompatible change or not. It's making the linter stricter, but I'd only expect new errors to be reported in code that's already breaking the rules of hooks. There is a potential edge-case of code with a forwardRef-wrapped component using a non-hook function with a name starting with "use"; this change would now return a false positive.