-
Notifications
You must be signed in to change notification settings - Fork 350
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
fix(ClipboardCopy): clear timer on unmount #6708
Conversation
Signed-off-by: Boaz Shuster <boaz.shuster.github@gmail.com>
@@ -111,6 +111,12 @@ export class ClipboardCopy extends React.Component<ClipboardCopyProps, Clipboard | |||
} | |||
}; | |||
|
|||
componentWillUnmount = () => { | |||
if (this.timer) { | |||
window.clearTimeout(this.timer); |
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.
Great catch!
Does this mean that the this.timer = null;
on line 204 and 357 is unnecessary?
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.
@nicolethoen that's a great question.
These lines were meant for a different case:
- If the user clicked on the copy button we set a timer that on timeout we will close the "copied" popover
- but what happens if the user clicks again on the copy button and the timer didn't timeout. A new timer will be created unnecessarily and we will have a flaky behavior in the popover display (it will be closed after
switchDelay
from the first click). - So what we did is check if there is a timer. Stop it and release it from the memory. and run a new timer.
In other words, these lines are necessary for a behavior of the popover and not covering the case where the component is unmounted and after the timeout we are changing the component state (we "cannot" change its state because it's not mounted!).
I hope I explained that well.
Thank you for the question - it was fun. 😄
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.
Fabulous explanation! Thank you so much!
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.
LGTM
What: Closes #6649