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 Defintions: Clock returned from install is missing uninstall property/function #356

Closed peterjuras closed 3 years ago

peterjuras commented 3 years ago

FakeTimers version : 7.0.2 Environment : Node.js Example URL : https://codesandbox.io/s/sharp-fast-7zwis?file=/src/index.ts Other libraries you are using: TypeScript (4.1.3)

What did you expect to happen?

When storing the return value of install in a variable, the uninstall function helps uninstalling the fake clock again from the environment.

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

const clock = FakeTimers.install();

clock.uninstall();

Note: I'm currently limited in time, otherwise I would submit a PR which adds the types from DefinitelyTyped as an initial (tested and working) version for the types.

What actually happens

When using TypeScript, the return value of install does not include the uninstall property.

How to reproduce

You can check an example here: https://codesandbox.io/s/sharp-fast-7zwis?file=/src/index.ts

squareloop1 commented 3 years ago

It also seems to miss some other properties like tick() and tickAsync().

Due to the nature the types are defined it is also not possible to declare the type of a variable without installing it too:

// not possible for later installation e.g. in a beforeEach(), because there is no type definition name "FakeClock" or any other:
let fakeTimer: FakeClock; 

// have to do it like this and install it immediately. It is returning the type object directly instead of a named type referencing the object like in the example above.
const fakeTimer: FakeTimers.install(); 
peterjuras commented 3 years ago

@squareloop1

You can use ReturnType to get the type of the return value of .install() without installing it immediately, e.g.:

let fakeTimer: ReturnType<typeof FakeTimers.install>

However, having a dedicated Clock type similar to the one that the DefinitelyTyped type definitions would be preferable imho.

@fatso83: Is there a rationale behind generating types from JSDoc definitions? Is there anything that triggered this move away from making TypeScript users use the DefinitelyTyped types?

In my opinion you will be fighting an up-hill battle and have more maintenance effort with trying to make JSDoc types work. While I haven't looked deeply into how JSDoc comments are translated into TypeScript type definitions, I can imagine that the result will be inferior to manually written definitions (like from DefinitelyTyped) or type definitions that are generated from TypeScript code.

fatso83 commented 3 years ago

I never minded the external definitions (I even fixed a few errors in the sinon types), but there was a majority of the team in favor of it, and the reasoning was partly to do with increasing internal documentation, as well. Assuming we are able to get a 1:1 feature parity with TS from the JS docs, I think this makes sense, but of course: if there is stuff that is hard to represent using JSDoc we might need to rethink.

As we are not going to redo these projects in TypeScript, but we do want better docs, this has the potential of generating types that are up to date as the project hums along. When I have looked at various definitions that have been generated in the DT packages, I have seen definitions for APIs that we have long since deprecated and removed, and docs generated directly from the package would have a better change of taking care of that. The docs would also have the upside of staying in sync with the version that is used, which is non-trivial using DT.

Is there a rationale behind generating types from JSDoc definitions? Is there anything that triggered this move away from making TypeScript users use the DefinitelyTyped types?

@mroderick You are better equipped to answer this.

SimenB commented 3 years ago

Should probably set up tsd or something like it so tests can be written for the types.

Some feature parity with DT should probably have been aimed for before releasing, but what's done is done 😀

fatso83 commented 3 years ago

tsd seems like a great package! Wonder why that is not used in the DT sub projects.

mroderick commented 3 years ago

Is there a rationale behind generating types from JSDoc definitions?

See:

mroderick commented 3 years ago

Is there anything that triggered this move away from making TypeScript users use the DefinitelyTyped types?

We can make up to date type definitions available to the TS community and save effort.

Perhaps we jumped the gun and released the files too soon. I'm hoping that the TS/DT community will see this in a kind light, and help bring us up to parity with the DT types by contributing to the documenation effort

For those that have gotten bitten by the immature .d.ts files: there's documentation on how to use the DT types in your project

bodinsamuel commented 3 years ago

As we are not going to redo these projects in TypeScript

Is this something you are strict on this? I mean the file is not too big, we could rewrote it in TS, not strictly, without big issues. Not saying it's easy, but it's doable.

mantoni commented 3 years ago

@bodinsamuel Yes, we're strict on this. It's not about effort or file size.