Skip to content
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

Add timeout utility function #114

Merged
merged 1 commit into from
Jul 5, 2024
Merged

Add timeout utility function #114

merged 1 commit into from
Jul 5, 2024

Conversation

nickschot
Copy link
Member

This PR adds the timeout utility function. It also updates all the tests, example code and examples to use it.

Closes #111

@beerinho
Copy link
Collaborator

What do you think about testing the timeout? Both options seem a bit strange, but thankfully Vitest has time faking built in - https://vitest.dev/api/vi.html#fake-timers - so hopefully it'd be quite straightforward

@nickschot
Copy link
Member Author

What do you think about testing the timeout? Both options seem a bit strange, but thankfully Vitest has time faking built in - https://vitest.dev/api/vi.html#fake-timers - so hopefully it'd be quite straightforward

Yeah so currently it's indirectly tested through all other tests. Testing it directly should be possible (without fake timers) though. I'll add something.

@nickschot
Copy link
Member Author

What do you think about testing the timeout? Both options seem a bit strange, but thankfully Vitest has time faking built in - https://vitest.dev/api/vi.html#fake-timers - so hopefully it'd be quite straightforward

Added some tests, 1 without fake timers (since I wanted to verify the microtick) and 1 with fake timers (to check the longer duration).

Copy link
Collaborator

@paoloricciuti paoloricciuti left a comment

Choose a reason for hiding this comment

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

Very cool...i'm wondering if we should expose utils as an export instead of "polluting" the main export. We could also export didCancel from there.

WDYT? Feel free to merge if you think it should be from the main one or refactor and merge otherwise! 🚀

@nickschot
Copy link
Member Author

Very cool...i'm wondering if we should expose utils as an export instead of "polluting" the main export. We could also export didCancel from there.

WDYT? Feel free to merge if you think it should be from the main one or refactor and merge otherwise! 🚀

We didn't seem to have a very clear strategy about that yet, let's discuss during the sync.

@nickschot nickschot merged commit 6407341 into main Jul 5, 2024
2 checks passed
@nickschot nickschot deleted the timeout branch July 5, 2024 07:36
@github-actions github-actions bot mentioned this pull request Jul 5, 2024
@github-actions github-actions bot mentioned this pull request Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add timeout utility
3 participants