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

Fix JSDoc for createClock #381

Merged
merged 1 commit into from
May 27, 2021
Merged

Conversation

henhal
Copy link
Contributor

@henhal henhal commented May 27, 2021

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.

This improves the generated .d.ts so that createClock may be called without arguments.
@codecov
Copy link

codecov bot commented May 27, 2021

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

henhal commented May 27, 2021

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

henhal commented May 27, 2021

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

fatso83 commented May 27, 2021

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 fatso83 merged commit 6101769 into sinonjs:master May 27, 2021
@fatso83
Copy link
Contributor

fatso83 commented May 28, 2021

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: #220 (comment)

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

fatso83 commented Jun 1, 2021

This is related to ☝️ : #386

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

Successfully merging this pull request may close these issues.

2 participants