[perf] Prevent unneeded clearTimeout calls
#39060
Conversation
romgrk
added
performance
package: utils (private)
| export { default as unstable_useLazyRef } from './useLazyRef'; | ||
| export { default as unstable_useTimeout, Timeout as unstable_Timeout } from './useTimeout'; | ||
| export { default as unstable_useOnMount } from './useOnMount'; |
There was a problem hiding this comment.
I would have released these without the unstable_ prefix but I see that all your hooks use this format so I've kept them consistent with that.
There was a problem hiding this comment.
@mnajdova should we plan to move away from this naming strategy? I don't know why/when we started with it
There was a problem hiding this comment.
We had this whole conversation about how we want to treat this package. We used unstable_ to allow ourselves to do breaking changes whenever we need. There were discussions that we may want to keep this package in alpha all the time, this would mean that Base UI, Material UI and other packages will re-export the things that makes sense, or make sure the APIs are consistent. @ would tag @michaldudak as a owner of the @mui/utils package to share his opinion.
There was a problem hiding this comment.
Is this package used externally? It doesn't seem to be publicized, doesn't have a readme on npm and doesn't seem to have docs. If it's not used externally, there shouldn't be any downsides to publishing a new major version each time we need to make changes. I must say it's a bit annoying to have to destructure the unstable_ prefix each time it's used in the datagrid.
For the hooks that I've introduced here, I also don't see any potential breaking change in the future. Their behavior is pretty clear and fundamental, e.g. I don't see how useLazyRef could be expanded in a breaking way.
There was a problem hiding this comment.
I added it for discussion on Monday's Core weekly. Let us know if you wish to attend to discuss this so we can send an invite @romgrk 😊
Netlify deploy previewhttps://deploy-preview-39060--material-ui.netlify.app/ @material-ui/core: parsed: -0.08% 😍, gzip: +0.03% Bundle size reportDetails of bundle changes (Toolpad) |
github-actions
bot
added
the
PR: out-of-date
github-actions
bot
removed
the
PR: out-of-date
There was a problem hiding this comment.
The useTimeout hook is a great improvement over using timeouts directly 👏🏼
I left a question for you and one "off topic" for @mnajdova
| export { default as unstable_useLazyRef } from './useLazyRef'; | ||
| export { default as unstable_useTimeout, Timeout as unstable_Timeout } from './useTimeout'; | ||
| export { default as unstable_useOnMount } from './useOnMount'; |
There was a problem hiding this comment.
@mnajdova should we plan to move away from this naming strategy? I don't know why/when we started with it
| const closeTimer = useTimeout(); | ||
| const enterTimer = useTimeout(); | ||
| const leaveTimer = useTimeout(); | ||
| const touchTimer = useTimeout(); |
There was a problem hiding this comment.
Looks good to me 🚀 Nice improvement
@brijeshb42 @michaldudak any comments before merging?
Follow-up of #37512 (comment)
Notion task page
Prevent unneeded usages of
clearTimeoutby introducing auseTimeouthook and aTimeoutclass that encapsulate the logic required to avoid those calls, as well as handle effect cleanup automatically.The hooks introduced here are already used in the datagrid, once this is merged I'll clean them up from there to use these.