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 initial delay support + Version bump #21

Closed
wants to merge 3 commits into from

Conversation

jamesst20
Copy link

@jamesst20 jamesst20 commented Apr 24, 2021

Give the ability tot have a custom initial delay. It can be useful in a lot of cases and it would allow us to also run immediately an interval instead of waiting the interval in the first place.

I ensured there would not be any breaking changes or signature changes.

How to use

setIntervalAsync (() => {}, 1000, ...args) // Initial + After delay is 1000ms
setIntervalAsync (() => {}, [0, 1000], ...args) // First run after 0ms and then every 1000ms

What's next:

TypeScript signature will need to permit the array. In the mean time it is not breaking anything because the original signature is 100% compatible.
https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/set-interval-async

interval: number
should become
interval: number|[number, number]

@jamesst20
Copy link
Author

jamesst20 commented May 2, 2021

@ealmansi

Is there a chance you can have a look ? I'm still using the insecure wrapper from the issue I created in my project and I would love to not have an "insecure" wrapper when my app will be ready for production

@jamesst20
Copy link
Author

@ealmansi Still waiting to get an answer, it has been over a month. Any chance to get feedback?

@jamesst20
Copy link
Author

@ealmansi Any update? :)

@ealmansi
Copy link
Owner

ealmansi commented Aug 3, 2021

Hi @jamesst20 , thanks for your contribution and apologies for the slow reply.

I'm closing the PR because the proposed solution is IMHO not good API design. As a user of this library, it would be entirely unintuitive to me why I would use an array instead of a number for the interval argument, and what the expected behavior of doing so would be. It feels like a hack.

There's a couple of additional issues with the PR:

  • There's a bug in the validation logic for the case where the interval argument is an array of length 0 or 1 (we're comparing against undefined, even though the result would still be the expected one).
  • Actual tests would be required to check that the function is behaving as expected when an array is passed in - this case is not properly covered by the existing test suite.

@ealmansi ealmansi closed this Aug 3, 2021
@jamesst20
Copy link
Author

Hi @jamesst20 , thanks for your contribution and apologies for the slow reply.

I'm closing the PR because the proposed solution is IMHO not good API design. As a user of this library, it would be entirely unintuitive to me why I would use an array instead of a number for the interval argument, and what the expected behavior of doing so would be. It feels like a hack.

There's a couple of additional issues with the PR:

  • There's a bug in the validation logic for the case where the interval argument is an array of length 0 or 1 (we're comparing against undefined, even though the result would still be the expected one).
  • Actual tests would be required to check that the function is behaving as expected when an array is passed in - this case is not properly covered by the existing test suite.

If you have a better idea in mind I am open. Also, the usage of an array was optional which I think kind of invalidate a bit the "unintuitive" part.

@arthur-caillaud
Copy link

Would love to see this PR merged

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.

None yet

3 participants