-
-
Notifications
You must be signed in to change notification settings - Fork 309
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
Remove unused hook #2671
Remove unused hook #2671
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😂💪🏻
@@ -1,17 +0,0 @@ | |||
import * as React from 'react'; | |||
import useEvent from './useEvent'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useEvent
looks almost identical to @mui/utils/useEventCallback
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a direct implemention of https://github.com/reactjs/rfcs/blob/useevent/text/0000-useevent.md
the idea was to one day replace it with the built-in, but the RFC got abandoned
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an opportunity to merge these two helpers? But I'm not sure https://github.com/mui/mui-toolpad/blob/0c05f1a5d655f711b704f04b3f9998a9422f776c/packages/toolpad-app/src/utils/useEvent.ts looks a bit more bundle size and none React 17 compatible. I didn't know about the useInsertionEffect
hook, interesting.
I linked the RFC in mui/material-ui@89635d1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an opportunity to merge these two helpers?
We could use useEventCallback
in Toolpad. I'm just curious, what is an enhanced effect? And is there an opportunity to better name/document this hook?
I didn't know about the
useInsertionEffect
hook, interesting.
The RFC polyfill mentions an effect with comment:
// In a real implementation, this would run before layout effects
useLayoutEffect(() => {
Which is what useInsertionEffect
does. In practice we can probably rely on useLayoutEffect
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just curious, what is an enhanced effect? And is there an opportunity to better name/document this hook?
A use layout effect but without the server side hydration warning. Yeah, the name is subjective 😁. The name used also impacts the eslint rule, e.g. facebook/react#25285. We would need to update https://github.com/mui/material-ui/blob/89635d1dd83b9533e5853a287df014f10f07f90f/.eslintrc.js#L123 too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick google shows mostly useIsomorphicLayoutEffect
being used as a name. I'm just mostly interested in adding a comment there. Just a few extra points.
- I'll switch to
useEventCallback
in Toolpad - With this
useEnhancedEffect
hook, I'd be a bit concerned. It's being applied correctly in the case ofuseEventCallback
. But in many cases the warning could be justified and using this hook would be sweeping the problem under the carpet. - It's being redefined in some of the docs examples, e.g. https://github.com/mui/material-ui/blob/135d00a4091b3d9ff867ec8488a5a64aa5d33d21/docs/data/joy/customization/theme-colors/RemoveActiveTokens.js#L26. Perhaps this should also import the
@mui/utils
one? (The usage here seems a bit funky as well) - [core] Add a comment to explain
useEnhancedEffect
material-ui#39035
Removing some dead code.
useRisingEdge
was a hook to run an effect when a value changes fromfalse
totrue
.