[core] Make useForkRef variadic #27939
Conversation
| }; | ||
| }, [refA, refB]); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
That's why it shouldn't be variadic.
There was a problem hiding this comment.
This warning is caused by the inability of the tool to verify if I passed all the dependencies. Since I use all the parameters provided to useForkRef, I know all of them should be in the dependencies (facebook/react#14920 (comment)).
Alternatively, if this really is a no-go for you, would a 4-parameter version of this function be more acceptable (it should cover all cases)?
function useForkRef<Instance>(
refA: React.Ref<Instance> | null | undefined,
refB: React.Ref<Instance> | null | undefined,
refC?: React.Ref<Instance> | null | undefined,
refD?: React.Ref<Instance> | null | undefined,
)There was a problem hiding this comment.
But spreading is all of the sudden valid e.g. useForkRef(...refs).
There was a problem hiding this comment.
Why is this not desirable, though?
There was a problem hiding this comment.
Because it breaks the rules of hooks
There was a problem hiding this comment.
I reverted to the original version and added two optional parameters. Please take a look.
There was a problem hiding this comment.
I don't think you want to see my point. Abandoning this discussion.
There was a problem hiding this comment.
Would I ask for it if I didn't want to? For me, the point of code review is to discuss the solution and highlight its good and bad sides.
If you think the solution sucks, I'm ok with that, as long as you explain your reasoning. This way I can either counter your arguments or simply learn something about the existing codebase and patterns.
github-actions
bot
added
PR: out-of-date
|
Reopening as discussed in #34383 |
michaldudak
force-pushed
the
variadic-forkRef
branch
from
2812477 to
b510628
Compare
michaldudak
requested review from
mnajdova,
siriwatknp and
danilo-leal
as code owners
| @@ -0,0 +1,110 @@ | |||
| import * as React from 'react'; | |||
There was a problem hiding this comment.
This was extracted from mui-material/utils/index.test.js and moved as-is here.
| @@ -0,0 +1,38 @@ | |||
| import * as React from 'react'; | |||
There was a problem hiding this comment.
This was extracted from mui-material/utils/index.test.js and moved as-is here.
| @@ -0,0 +1,29 @@ | |||
| import * as React from 'react'; | |||
There was a problem hiding this comment.
This is what remained in mui-material/utils/index.test.js after moving useForkRef and setRef out of this file.
michaldudak
force-pushed
the
variadic-forkRef
branch
from
b510628 to
5bd1ce1
Compare
| }; | ||
| }, [refA, refB]); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
Can you explain a bit more why it is needed even though the refs is used as dependencies? Is it because eslint can't detect this use case?
The implementation looks good to me.
There was a problem hiding this comment.
This ESLint rule requires an array literal to be passed in as it can't statically verify the correctness of the code otherwise.
This allows useForkRef to be called with more than two parameters, eliminating the need for one-time use variables and improving readability of the code.
Closes #34383