sinonjs / fake-timers

Fake setTimeout and friends (collectively known as "timers"). Useful in your JavaScript tests. Extracted from Sinon.JS
BSD 3-Clause "New" or "Revised" License
797 stars 104 forks source link

Type definitions introduced in v7 are too strict #352

Closed peterjuras closed 3 years ago

peterjuras commented 3 years ago

What did you expect to happen?

The type definitions that have been introduced in v7 appear to be too strict and don't allow users to leave out properties that have default values.

As an example, the following code currently throws an error when using it with TypeScript:

import FakeTimers from "@sinonjs/fake-timers";

FakeTimers.install({
  now: 1
});

FakeTimers.install();

This is because the type definitions do not have question marks for optional properties or arguments. Function parameters and object properties can be marked as optional using a question mark, e.g.:

function example(requiredParam1: { requiredProp1: number; optionalProp1?: number }, optionalParam1?: number) {
  // ...
}

How to reproduce

You can check out the following codesandbox: https://codesandbox.io/s/headless-lake-jq53p?file=/src/index.ts

fatso83 commented 3 years ago

Quickest way to get this fixed is through a tiny PR with some ?s. You seem to know what is required ... ;)

vovkasm commented 3 years ago

It will not work. Those type definitions is totally broken ))) What install returns? The Clock object that can be used as a controller for "fake" time in tests. Clock has various functions for advance time: tick, runAll, etc... and to control "fake" time state: reset, setSystemTime... but nothing here )))

Currently it has:

{
    now: number;
    timeouts: {};
    Date: any;
    loopLimit: number;
}

Commit https://github.com/sinonjs/fake-timers/commit/0d093827e585769aeeda7420a7bcdf757f352e7d states:

In order to improve the experience of TypeScript users, we are compiling .d.ts files from the JSDoc and distributing them with the package

But in reality it makes the experience of TypeScript users much worse. Just compare generated from JSDoc types with manual typings from DefinitelyTyped: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/sinonjs__fake-timers/index.d.ts Before this change, we (TypeScript users) simply npm i -D @types/sinonjs__fake-timers and it was enough. Now we will need to make not trivial configuration to guide typescript to ignore native typings and use those from @types/sinonjs__fake-times...

And no, fix for this not "tiny PR"... )))

I fully support the idea to have typings with code in the same package, but not broken typings please!

fatso83 commented 3 years ago

@vovkasm We suggest users to stick with version 6 if they want to use the DT types. If you can help us out in improving them that would be great! Would you happen to know if there is a way of configuring TS to use the types from DT instead of the bundled ones? If so, we could add that to our docs (ref https://github.com/sinonjs/fake-timers/pull/355).