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

[useForkRef] Accept an array of refs #34383

Closed
2 tasks done
oliviertassinari opened this issue Sep 19, 2022 · 6 comments · Fixed by #27939
Closed
2 tasks done

[useForkRef] Accept an array of refs #34383

oliviertassinari opened this issue Sep 19, 2022 · 6 comments · Fixed by #27939
Labels
enhancement This is not a bug, nor a new feature package: utils Specific to the @mui/utils package priority: low To delay as much as possible

Comments

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 19, 2022

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

Summary 💡

Instead of:

const handleRef = useForkRef(
updateHostElementName,
useForkRef(externalRef, useForkRef(focusVisibleRef, buttonRef)),
);

we could have:

  const handleRef = useForkRef([updateHostElementName, externalRef, focusVisibleRef, buttonRef]);

Examples 🌈

The signature of

Motivation 🔦

It's an idea of simplification that I notice while looking at a notification on gregberge/react-merge-refs#5.

@oliviertassinari oliviertassinari added package: utils Specific to the @mui/utils package status: waiting for maintainer These issues haven't been looked at yet by a maintainer enhancement This is not a bug, nor a new feature priority: low To delay as much as possible labels Sep 19, 2022
@michaldudak
Copy link
Member

I've already attempted to do so in #27939.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Sep 20, 2022

@michaldudak It looks like it was a fun conversation. From what I understand, the discussion stopped at

it breaks the rules of hooks

I don't see how this statement is true. For example, the eslint rule says that It can't determine if the use case is valid or not, so its up to you to evaluate the case:

https://github.com/facebook/react/blob/b2748907c3cffb226447417957f2608a7c60c27d/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js#L2440-L2444

It seems OK.

@oliviertassinari oliviertassinari removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Sep 20, 2022
@kabernardes
Copy link
Contributor

Hey, can I try this one?!

@kabernardes
Copy link
Contributor

@oliviertassinari, I am looking at this issue and I have a question/suggestion:
I used the initial idea of @michaldudak in #27939, which appears to work. But, it will be a little different than what you ask, it would be:

const handleRef = useForkRef(updateHostElementName, externalRef, focusVisibleRef, buttonRef);

When I try to put all the refs in an array, it didn't accept because some can be undefined or null, and typeScript doesn't accept that.
Can the solution be like that? I believe it will improve the code the same way you want.

Thanks for your attention!

@michaldudak
Copy link
Member

michaldudak commented Oct 4, 2022

@oliviertassinari if you agree with my points in #27939, we can just merge that one. I think the usage will be even simpler than with an array.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Oct 4, 2022

@michaldudak The first approach you did in #27939 seems correct. From what I understand, the eslint error was about raising the attention that the plugin can't statically determine if it's a correct or not use of the hook API this way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is not a bug, nor a new feature package: utils Specific to the @mui/utils package priority: low To delay as much as possible
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants