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

[core] Replace a useCallback by useRef in useEventCallback #39078

Merged
merged 1 commit into from
Oct 22, 2023

Conversation

romgrk
Copy link
Contributor

@romgrk romgrk commented Sep 20, 2023

Minor touchup, useRef is leaner than useCallback.

@romgrk romgrk added performance package: utils (private) Specific to the private @mui/utils package labels Sep 20, 2023
@mui-bot
Copy link

mui-bot commented Sep 20, 2023

Netlify deploy preview

https://deploy-preview-39078--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 5a2490c

@oliviertassinari oliviertassinari changed the title [perf] Replace a useCallback by useRef [core] Replace a useCallback by useRef Sep 20, 2023
@oliviertassinari oliviertassinari changed the title [core] Replace a useCallback by useRef [core] Replace a useCallback by useRef in useEventCallback Sep 20, 2023
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM cc @michaldudak

@romgrk romgrk merged commit fdfaf64 into mui:master Oct 22, 2023
21 checks passed
@romgrk romgrk deleted the perf-use-event-callback branch October 22, 2023 17:40
@oliviertassinari
Copy link
Member

This makes me think of this thread: mui/mui-toolpad#2671 (comment) I had with @Janpot. We initially copied https://github.com/reactjs/rfcs/blob/useevent/text/0000-useevent.md#internal-implementation

@romgrk
Copy link
Contributor Author

romgrk commented Oct 25, 2023

Tbh I feel our useEventCallback is a bit weird. In the React RFC you linked, the ref is initialized as null under the rationale that they want it to throw if it's called during render, for reasons that make sense from a correctness/DX POV (even if a bit pedantic, but I get they need it in react core). But in our implementation we both initialize the ref with the callback directly AND we update it in the useLayoutEffect. So we both don't get the benefit of throw-during-render, and we pay the cost of adding an effect.

@Janpot
Copy link
Member

Janpot commented Oct 30, 2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: utils (private) Specific to the private @mui/utils package performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants