[Snackbar] Fix timer restarting when parent component re-render #18361
Conversation
Details of bundle changes.Comparing: f0bfa93...9bd6e44
|
|
@weslenng Could you add a test case? Thanks. |
oliviertassinari
added
bug 🐛
|
@oliviertassinari I will try to make a test case, but as I had questioned in PR I don't know exactly how to test this situation. Can you help me? |
|
I would recommend to start from the codesandbox reproduction. Convert it to a test case, observe that it fails without the patch. This should do it :). |
|
@oliviertassinari Shouldn't this test work? The logic makes sense to me, but it's not failing (without the patch). it('should call onClose when timer done after a parent re-render', () => {
const handleClose = spy();
const autoHideDuration = 2e3;
const TestCase = props => (
<div>
<div>{props.renderParent ? "Hello" : "World"}</div>
<Snackbar
open={props.open}
onClose={handleClose}
message="message"
autoHideDuration={autoHideDuration}
/>
</div>
);
const wrapper = mount(<TestCase />);
wrapper.setProps({ open: true });
assert.strictEqual(handleClose.callCount, 0);
clock.tick(autoHideDuration / 2);
wrapper.setProps({ renderParent: true });
clock.tick(autoHideDuration / 2);
assert.strictEqual(handleClose.callCount, 1);
assert.deepEqual(handleClose.args[0], [null, 'timeout']);
}) |
|
@weslenng The onClose reference needs to change between two renders. |
| }, | ||
| [autoHideDuration, onClose], | ||
| ); | ||
| const setAutoHideTimer = useEventCallback(autoHideDurationParam => { |
There was a problem hiding this comment.
This uses a layout effect internally which is not necessary I believe. Looking at how useInterval is defined I think we can use useEffect or better extract this into a separate useTimeout hook so that it is crystal clear what is responsible for what.
https://overreacted.io/making-setinterval-declarative-with-react-hooks/#just-show-me-the-code
oliviertassinari
added
the
PR: needs revision
Thanks. @oliviertassinari The case test is done. In which prop describe do you think it would be semantically correct? I'm between |
|
@weslenng I would say |
oliviertassinari
removed
the
PR: needs revision
There was a problem hiding this comment.
I have updated the pull request to also use the latest onClose prop. I have updated the tests to reflect it. I have explored the useTimeout approach, the problem is that it would require to rerender when the timer is pause or resume, it's unclear how we can handle the resumeHideDuration vs autoHideDuration difference down this path. I haven't changed the imperative approach, I think that we can revisit it in the future if it cause a problem. I think that it's good enough to be merged.
|
@weslenng Thanks. Especially for adding a test 👍 |
How can I create tests for this fix?
(Closes #18353)