Change lolex.install signature #103
Conversation
|
I think that leaving the possibility for |
|
I see your point, but I dunno ... IMHO if we break the signature, we might as well make it consistently break, not just surprisingly break for some of the combinations. |
|
@mroderick do you have any input on this? there was a sudden influx of changes, and so I thought this might be a good time to add this before accepting some of them. |
| /** | ||
| * @param config {Object} optional config | ||
| * @param config.target {Object} the target to install timers in (default `window`) | ||
| * @param config.now {number|Date} a number (in milliseconds) or a Date object (default epoch) |
There was a problem hiding this comment.
Is this parameter optional? Should it have a default value?
There was a problem hiding this comment.
All the current parameters are optional, lolex.install() is used in the README, so this behaviour-preserving and should not break anything.
We could add more explicit defaults of course, before sending them on to createClock and such (that have their own defaults), but I felt that would perhaps obfuscate my intent in this PR. Could add that in another commit, of course.
There was a problem hiding this comment.
Faking time is confusing enough as it is, let's not change the api too much :)
| var clock = createClock(now, loopLimit); | ||
| var i, l; | ||
| var target = config.target || global; | ||
| var clock = createClock(config.now, config.loopLimit); |
There was a problem hiding this comment.
What happens if config.now is undefined?
There was a problem hiding this comment.
The same as today. It is converted to 0 down the line.
I am not a super-fan of the current digging-down-into-the-code to see if not passing a number is allowed or not, so adding defaults higher up along with strict checking further down, is more my style.
|
|
|
Not totally sure what the squirrel with hat means ... Is that an approval? 😂 |
|
I am happy with this PR, merge it! |
update lolex.install: sinonjs/fake-timers#103
As shown in #102, something as simple as adding one additional configuration option becomes unmanageable due to the current design of a "telescoping method signature" where all the arguments are optional. This makes contributors reach for alternate ways of configuring lolex, such as adding flags on the object.
I propose we change the signature to accept a single configuration object with named parameters to simplify this. This PR is that proposal implemented. There are also many other pros to constructor-like functions like this of 1-arity.
This is a breaking change and will require a new major version.