Skip to content

Conversation

@jquense
Copy link
Member

@jquense jquense commented Jan 20, 2020

No description provided.

@jquense jquense requested a review from taion January 20, 2020 21:22
Copy link
Member

@taion taion left a comment

Choose a reason for hiding this comment

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

do you need to expose a non-hook version?

@jquense
Copy link
Member Author

jquense commented Jan 20, 2020

i don't currently need to...at least i don't think so...

@taion
Copy link
Member

taion commented Jan 20, 2020

okay, sorry, now that i'm looking at this again... this is actually going to have a very slightly wrong timeout because we're not accounting for drift in firing the timeout

this probably makes no practical difference, but it wasn't a problem in our internal implementation because we were skipping the setTimeout abstraction entirely and just calculating from the intended expiration time

would it make sense here to calculate Date.now() + ms, and keep trying to schedule toward that time?

@jquense
Copy link
Member Author

jquense commented Jan 21, 2020

added accounting for drift, tho i did it super quickly so plz check my work

@taion
Copy link
Member

taion commented Jan 22, 2020

@jquense tried to clean up the logic a bit; let me know what you think

@jquense
Copy link
Member Author

jquense commented Jan 22, 2020

LGTM!

@jquense jquense merged commit 32d6951 into master Jan 22, 2020
@jquense jquense deleted the safe-timeout branch January 22, 2020 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants