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

Type Defintions: Clock returned from install is missing uninstall property/function #356

Closed
peterjuras opened this issue Jan 19, 2021 · 9 comments · Fixed by #365
Closed

Comments

@peterjuras
Copy link

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
Copy link

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
Copy link
Author

peterjuras commented Jan 27, 2021

@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
Copy link
Contributor

fatso83 commented Jan 27, 2021

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
Copy link
Member

SimenB commented Jan 27, 2021

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
Copy link
Contributor

fatso83 commented Jan 27, 2021

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

appsemble-bot pushed a commit to appsemble/appsemble that referenced this issue Jan 27, 2021
@mroderick
Copy link
Member

Is there a rationale behind generating types from JSDoc definitions?

  • The Sinon libraries need better API documentation
  • API documentation can be generated from JSDoc
  • JSDoc can be integrated into our static analysis (eslint-plugin-jsdoc), which helps keep documentation up to date with the source
  • Modern IDEs and editors scans the JS source files and reads JSDoc to provide usage hints (even VSCode in JS projects)
  • Bonus Up to date type definitions can be generated from JS source

See:

@mroderick
Copy link
Member

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
Copy link

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
Copy link
Member

mantoni commented Feb 8, 2021

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

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

Successfully merging a pull request may close this issue.

7 participants