-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Components: Fix the Snackbar auto-dismissal timers #58604
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core SVNIf you're a Core Committer, use this list when committing to
GitHub Merge commitsIf you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
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.
Could also add a CHANGELOG entry? Thank you!
// The `onDismiss/onRemove` can have unstable references, | ||
// trigger side-effect cleanup, and reset timers. | ||
const callbackRefs = useRef( { onDismiss, onRemove } ); | ||
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.
What's the reason for useLayoutEffect
instead of useEffect
? In theory useEffect
s should be called in order, so we should be guaranteed that the onDimiss
and onRemove
refs are always up to date
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.
Nothing specific. This is just a common pattern; we even have a useLatestRef
hook, but I don't use here because returned value isn't recognized as Ref and linter complains.
Related: https://epicreact.dev/the-latest-ref-pattern-in-react/
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.
Thank you for the link, super interesting read!
I took a look back at this article by Kent that I usually refer to when talking useEffect
and useLayoutEffect
, I noticed the "special case" section 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.
🚀
Code changes LGTM and tests well as per instructions.
I also realised that we don't have unit tests for this component, which would have been handy here — but not in the scope for this PR.
Thank you for working on this, @Mamaduka 🙏
Thanks for testing and review, @ciampo! |
What?
Resolves #58594.
PR removes
onDismiss/onRemove
callbacks from side-effect dependencies to avoid resetting timers on re-render.Why?
See #58594.
Testing Instructions
Expected behavior
Let's assume there is a one-second difference between message triggers.
Screenshots or screencast