setTimeout fires immediately if delay is a (numeric) string

[acoulton]

• FakeTimers version : 6.0.0
• Environment : any
• Example URL : https://jsbin.com/jijuyayezo/edit?js,console
• Other libraries you are using: N/A

What did you expect to happen?

Calling setTimeout(callback, '1000') should use a delay of 1000ms. E.g. if the delay has been parsed from a data-attribute or other stringy option value.

What actually happens

The delay is ignored and it's treated as an immediate timeout. This differs from browser behaviour.

It appears to be because addTimer first checks isNumberFinite to determine whether to respect the delay or treat it as an immediate callback. And isNumberFinite always returns false if the value provided is not an explicit number type.

To be consistent with browser behaviour it probably needs to attempt to cast a string value to a number - maybe e.g. if /^\d+$/.match(num)? However I'm not sure the best place in the codebase to do that and whether it would have other side effects.

How to reproduce

See JSBin, including verification of native behaviour. Or:

   var clock = FakeTimers.createClock();

    clock.setTimeout(
        function () { console.log('fake timer using string delay'); },
        '1000'
    );
    clock.setTimeout(
        function () { console.log('fake timer using int delay'); },
        1000
    );

    console.log('Ticking once, expect nothing to happen');
    clock.tick(1);
    console.log('Ticking 999 times, expect both timeouts to fire');
    clock.tick(999);

Observe the string delay timer fires after the first tick, not the second.

[fatso83]

Your guess is as good as mine - I would just try to add it at one place and run the test suite to see if anything failed :sweat_smile: If the standard uses type coercion, I guess it should be fine to simply do Number(timer.delay) at the right place; the earlier the better (by re-assigning timer.delay to a number)

My first guess was at the point of function definition for setTimeout, but that seemed too complex, so I am not quite sure.

[benjamingr]

Yes, this is a bug, we had to fix this sort of thing in bluebird. A fix would just be casting it to Number which is pretty much what the spec does.

This line here: https://github.com/sinonjs/fake-timers/blob/master/src/fake-timers-src.js#L276-L278 just needs a typeof !== 'number' check and if it's not to convert it. The check here: https://github.com/sinonjs/fake-timers/blob/master/src/fake-timers-src.js#L74-L76 needs to be removed.

If you want to work on a PR @acoulton go ahead - otherwise I'll whip something up.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[benjamingr]

Ok, I'll take that as a no and offer this fix to some interested parties and if they decline in 48 hours I'll make a PR :]

[arielweinberger]

There you go: https://github.com/sinonjs/fake-timers/pull/322

[benjamingr]

Thanks @arielweinberger - left a comment there.

[acoulton]

Whoops - sorry @benjamingr I had intended to take a look at this and then got knocked a bit sideways by COVID fallout... Thanks for picking it up @arielweinberger

[benjamingr]

This will be fixed in the next release :]