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

Remove generated typescript definitions #386

Closed fatso83 closed 3 years ago

fatso83 commented 3 years ago

Purpose (TL;DR) - mandatory

Remove generated Typescript definitions

Background (Problem in detail) - optional

When adding JSDoc to our functions our main points were

  1. Give us documentation, API docs
  2. Give autocomplete and type suggestions in supporting IDEs

As an added bonus, we found that it was possible to generate TypeScript definition files directly from the JSDoc definitions. There were already community efforts at DefinitelyTyped for both sinon and its sub-projects, like fake-timers, but as external projects they suffered from being out-of-sync with the projects they were supposed to describe. Our thoughs were then that by generating these directly from the source that we could in time catch up with the DT efforts and be the main source of up-to-date .d.ts files.

Unfortunately, there were some aspects of our types that were hard to describe statically using JSDoc, such as those where TypeScript would utilize its typeof type operator. That resulted in contributions that used the fact that TSC ignored invalid type definitions to describe types using those TypeScript features. The result was valid d.ts files, but invalid JSDoc, meaning our two main points were unmet. JSDoc simply is not powerful enough to generate the types expected by the users of the DefinitelyTyped types. So, in order to get back to what we mainly want, we stop shipping TypeScript definitions and continue to let this be a community maintained effort at DT (as it was). This should result in less friction (as experienced in the last few months), but will, of course, have the downside of always lagging a bit behind.

codecov[bot] commented 3 years ago

Codecov Report

Merging #386 (e52e2c2) into master (843e307) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #386   +/-   ##
=======================================
  Coverage   93.46%   93.46%           
=======================================
  Files           1        1           
  Lines         551      551           
=======================================
  Hits          515      515           
  Misses         36       36           
Flag Coverage Δ
unit 93.46% <ø> (ø)

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

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

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 843e307...e52e2c2. Read the comment docs.

SimenB commented 3 years ago

I do not understand why we cannot write the source of this module in TS, which should make sure the source is nice to use in IDEs whether or not you use TS (which from what I gather is the purpose of jsdoc) without that pain of going through type annotations in comments. When brought up the answer has been "no", but without any proper reasoning.

The source of this library is pretty stable already (barring bug fixes and new timing APIs (although the incoming temporal API might be some work)), and there is already a build step in place , so I don't think barrier of entry to contribution are particularly high?

(please tell me where to discuss this privately if anywhere and delete this comment if you want)

fatso83 commented 3 years ago

@SimenB I think that's a fair question, and I do not mind this being an open discussion. There are discussions happening outside of GitHub (we have a Telegram group for the core Sinon maintainers) where we discussed this a bit. This is @mroderick's take on this (he wrote a long time ago he would reiterate it in the next "migrate to TypeScript discussion", so I think it still stands 😄):

I really don't care for TS. I do care about having good documentation. JSDoc can give us good API documentation (with a lot of work). If we can get more people interested in writing documentation by giving them type definitions: everyone wins

There's simply not that much love to be found for TypeScript in itself among the various maintainers. And although we have asked for people to step up to do the maintenance work (merging PRs, updating deps, debugging and verifying various issues on the issue tracker), years later it's still mainly me, Morgan and Max doing it across the various projects. So whether the maintainers like working with TypeScript or just plain think it's a step in the wrong direction for JS matters in that sense.

The start of this was basically that we just wanted better API docs than what we had. And while yes, there is (was?) a build step already, TypeScript brings a lot else into the mix that adds a bit to the mental overhead. Me personally think TS has a lot of cool things going for it and I ~enjoy~ find it useful in the larger codebase I work with on a daily basis, but struggling to find the right incantations to appease the compiler is not how I enjoy spending my weekends 😄 What I like about contributing to these projects is the low threshold to fix something. There's no build step needed, no source maps to think about - you can run it in Node unaltered. And I think that's fine quality in itself.

That being said, this project is a bit different, in that you are one of the core maintainers (in @sinonjs/lolex-core) and has an interest in getting good quality TypeScript definitions for the Jest project. So with regards to the previous thinking of "the ones doing the work should decide" what you think definitely matters. I'm not religiously opposed to converting to TypeScript (not feeling to hot about it, though), pretty sure Benji is neither (given the work he has done at Testim), and the work Morgan does in this project is mostly not writing code, so with that in mind it could be different ... It would still be the odd man out, among our projects, and the core sinon project will definitely not be converted any time soon.

mroderick commented 3 years ago

I'm happy to have a TS discussion in a separate issue. This PR is about fixing what could have been good, but turned out it wasn't.