Skip to content
This repository was archived by the owner on Jan 10, 2025. It is now read-only.

Improve use timeout #1306

Merged
merged 3 commits into from
Apr 1, 2020
Merged

Improve use timeout #1306

merged 3 commits into from
Apr 1, 2020

Conversation

sambostock
Copy link
Contributor

@sambostock sambostock commented Mar 4, 2020 •
edited
Loading

Description

This updates the useTimeout hook in shopify/react-hooks to support the same API and behaviour as the useInterval hook added in #1241.

Specifically, it changes the behaviour of

  • callback: Previously, any time this prop changes, the timeout is reset. Now, the callback is free to change any time before the timeout fires, since the callback is stored in a ref.
  • delay: null can now be passed to clear the timeout.

Type of change

  • react-hooks Minor: The change to delay accepting null, in addition to numbers, as well as making changing the callback not reset the timeout.

Checklist

sambostock
sambostock commented Mar 4, 2020
View reviewed changes
packages/react-hooks/src/hooks/tests/timeout.test.tsx
Comment on lines +60 to +72
it('continues to respect the delay after the callback changes', () => {
const {callback, wrapper} = setup({delay: ONE_SECOND});

expect(spy).not.toHaveBeenCalled();
expect(newSpy).toHaveBeenCalled();
timer.runTimersToTime(HALF_A_SECOND);
expect(callback).not.toHaveBeenCalled();

const newCallback = jest.fn();
wrapper.setProps({callback: newCallback});

timer.runTimersToTime(HALF_A_SECOND);
expect(newCallback).toHaveBeenCalled();
expect(callback).not.toHaveBeenCalled();
});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this should be a breaking change or not. Technically, it does break the previous behaviour, though it would be weird behaviour to depend on, so I'm leaning towards breaking, but I want to confirm with reviewers.

Previously

Changing the callback prop causes the timeout to be reset, effectively restarting the timer. If the timeout has already fired, a new timeout will be set, and the new callback will be run (in addition to the one that already ran).

Now

Changing the callback does nothing. When the timeout fires, it will run whatever the current timeout is. If the timeout has already fired, nothing will happen, unless the delay is also changed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kinda see this as a bugfix tbh

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌 I've updated the Changelog to include two entries.

  • Added support for null delay
  • Fixed changing callback resetting the timeout

@sambostock sambostock marked this pull request as ready for review March 5, 2020 20:22
@sambostock sambostock force-pushed the improve-use-timeout branch from ca68568 to 8b6cafb Compare March 6, 2020 00:41
@GoodForOneFare GoodForOneFare requested review from a team and removed request for GoodForOneFare March 10, 2020 17:02
@GoodForOneFare
Copy link
Member

GoodForOneFare commented Mar 10, 2020 •
edited
Loading

Tagging in Web Foundation for review as I'm tied up in stuff.

@sambostock sambostock requested a review from cartogram March 13, 2020 16:53
marutypes
marutypes approved these changes Mar 30, 2020
View reviewed changes
Copy link
Contributor

@marutypes marutypes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me, I kinda think this should be positioned as a bugfix. I don't think the original behaviour was intended, as even the old test seemed worded in such a way that I would assume it would work the way it does now.

@sambostock
Copy link
Contributor Author

Thanks for the review @TheMallen!

I'm ok with the change to callback being considered a bugfix. Does the new behaviour of passing a null timeout to cancel (new backwards compatible feature) make it a minor version change then?

@sambostock sambostock force-pushed the improve-use-timeout branch from 8b6cafb to 651018b Compare March 30, 2020 19:01
@marutypes
Copy link
Contributor

@sambostock makes sense to me!

@sambostock sambostock force-pushed the improve-use-timeout branch 2 times, most recently from 7aad94c to bd06f6b Compare March 31, 2020 22:56
@sambostock sambostock force-pushed the improve-use-timeout branch from bd06f6b to b670692 Compare April 1, 2020 18:00
@sambostock sambostock merged commit 81317a6 into master Apr 1, 2020
@sambostock sambostock deleted the improve-use-timeout branch April 1, 2020 20:49
@alexandcote alexandcote requested a deployment to production April 2, 2020 17:30 Abandoned
@alexandcote alexandcote temporarily deployed to production April 2, 2020 17:30 Inactive
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cartogram cartogram Awaiting requested review from cartogram

1 more reviewer

@marutypes marutypes marutypes approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sambostock @GoodForOneFare @marutypes @alexandcote