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

Fix JSDoc for createClock #381

Closed henhal closed 3 years ago

henhal commented 3 years ago

This improves the generated .d.ts so that createClock may be called without arguments.

Purpose (TL;DR) - mandatory

Improve .d.ts types for createClock() so that it's possible to call it from TypeScript without arguments.

The currently generated .d.ts uses JSDoc, which incorrectly specified the two arguments for createClock() as mandatory.

codecov[bot] commented 3 years ago

Codecov Report

Merging #381 (cdd0391) into master (f5d9b3a) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #381   +/-   ##
=======================================
  Coverage   93.49%   93.49%           
=======================================
  Files           1        1           
  Lines         553      553           
=======================================
  Hits          517      517           
  Misses         36       36           
Flag Coverage Δ
unit 93.49% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/fake-timers-src.js 93.49% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f5d9b3a...cdd0391. Read the comment docs.

henhal commented 3 years ago

Authors: IMHO, generating .d.ts from JSDoc should be considered a very temporary approach. Is there a reason why this was introduced and put in favour of DefnitelyTyped? Now when importing the library it's harder to use than before, since the generated typings are not at all mature. The very first call I made was to createClock(), and I was confused by it requiring two parameters...

I'd love to help, but I noticed that there would be quite a lot of work to fix all the function types etc. To get this working by only improving the JSDoc is going to be harder than any other solution I think. Actually I don't know in which shape DefinitelyTyped is for this repo, but I'm not sure it's a step in the right direction to go from a community managed DefinitelyTyped repo to an auto-generated one that suffers from poor JSDoc conversion.

In my book, of course the best way would be to rewrite Sinon in TypeScript. Are contributions for that of interest?

The second best is probably to add the d.ts files to source control and manually improve them within this repository.

The third best is to rely on DefinitelyTyped.

The worst, IMHO, is the current approach...

henhal commented 3 years ago

Perhaps it can also be noted that the suggest approach for still using DefinitelyTyped does not work perfectly either. If modifying paths in tsconfig.json, my code compiles OK, and clicking on symbols from the library takes me to the DT repo. However, the IDE (Webstorm 2020) still links to the d.ts file in the source repo and refuses to understand that the code does compile.

This may be an IDE issue of course, but it still highlights that this library became much harder to use than before. I ended up downgrading to ^6.0.1. :(

fatso83 commented 3 years ago

Thanks for the PR! As to your comments, I'll leave that for now (as I'm in a hurry), but you absolutely raise some very valid points.

fatso83 commented 3 years ago

Is there a reason why this was introduced and put in favour of DefnitelyTyped?

Of course. If you are interested in the background you can find most of the reasoning in the discussion here: https://github.com/sinonjs/fake-timers/issues/220#issuecomment-430596244

go from a community managed DefinitelyTyped repo to an auto-generated one that suffers from poor JSDoc conversion.

We go from an external repo that is usually out of sync (I have contributed to the DT types several times when stuff I need at work is missing from DT) to one managed by the community and in-sync with our own docs. The main point for us is documenting the types used using JSDoc. The generated TypeScript definitions are a bonus.

In my book, of course the best way would be to rewrite Sinon in TypeScript. Are contributions for that of interest?

Not at all (would not mind a hand rewriting to ESM, though!), but not from bad will 😄 You need maintainers willing to keep it up to date in Typescript - of which there are none. It was hard enough getting contributors before TS was a thing. We have no interest in Babel, bundlers and source maps getting in our way when debugging - I think we all have enough battle scars from the last decade of fighting various generations of such machinery 😸

P.S. No personal grudges against TypeScript. We started using it at work before it was cool (v0.9) 😎

fatso83 commented 3 years ago

This is related to ☝️ : #386