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

Remove generated typescript definitions #386

Merged
merged 5 commits into from
Jun 8, 2021

Conversation

fatso83
Copy link
Contributor

@fatso83 fatso83 commented May 29, 2021

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.

Needed to get the eslint compat plugin to work
correctly
@fatso83 fatso83 marked this pull request as draft May 29, 2021 16:46
@codecov
Copy link

codecov bot commented May 29, 2021

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.

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.
@fatso83 fatso83 force-pushed the remove-typescript branch from e7cf67c to 119615b Compare May 29, 2021 16:54
@SimenB
Copy link
Member

SimenB commented May 29, 2021

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 fatso83 force-pushed the remove-typescript branch from a724edc to 011eafd Compare May 29, 2021 19:27
@fatso83
Copy link
Contributor Author

fatso83 commented May 29, 2021

@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.

@fatso83 fatso83 changed the title Remove typescript Remove typescript definitions May 29, 2021
@fatso83 fatso83 marked this pull request as ready for review May 29, 2021 23:04
@fatso83 fatso83 mentioned this pull request Jun 1, 2021
Copy link
Member

@mroderick mroderick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last commit should probably be "Fix invalid JSDoc".

@mroderick
Copy link
Member

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.

@fatso83 fatso83 changed the title Remove typescript definitions Remove generated typescript definitions Jun 8, 2021
@fatso83 fatso83 merged commit dbc606a into sinonjs:master Jun 8, 2021
@fatso83 fatso83 deleted the remove-typescript branch June 8, 2021 12:25
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.

3 participants