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

Remove convert timeout to number type in case typeof not string #39

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

spirinvladimir
Copy link

There are two tests which already cover this code base:
'should accept millisecond timeout'
'should accept string timeout'
Both passed - ok.

Copy link
Member

@UlisesGascon UlisesGascon left a comment

Choose a reason for hiding this comment

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

LGTM! The Number conversion seems unnecessary

@dougwilson dougwilson removed the tests label Aug 4, 2020
@dougwilson
Copy link
Contributor

We may want to make a different change, perhaps actually guard against non-numbers if we are not going to convert inputs into numbers. This is because this change is altering the input behavior vs removing dead code -- and tests in the test suite are imperfect, they only test what someone put in there :) As an example, calling with the argument [2000] would set a 2s timeout before and after this change, but the .timeout property on the error will be an array instead of a number.

@spirinvladimir
Copy link
Author

According jsdoc for timeout function and current tests there are supported types:

  • number
  • string

Let look at example Number([2000, 2000]) should set delay to NaN. Other example Number(['2000']) should set delay to '2000' (as string). Only array with first element type number won't be supported after this PR. What is a reason to support Array instances here at all?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants