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

🚀 Feature: TypeScript .d.ts type declarations #4154

Open
boneskull opened this issue Jan 15, 2020 · 19 comments
Open

🚀 Feature: TypeScript .d.ts type declarations #4154

boneskull opened this issue Jan 15, 2020 · 19 comments
Labels
area: integrations related to working with 3rd party software (e.g., babel, typescript) area: repository tooling concerning ease of contribution area: usability concerning user experience or interface core-team issues which must be handled by Mocha's core team nice to have enhancement proposal of low priority status: blocked Waiting for something else to be resolved type: feature enhancement proposal

Comments

@boneskull
Copy link
Contributor

AFAIK, it's recommended by both the TypeScript team and DefinitelyTyped (the people that publish everything under the @types scope on npm) that packages ship their own type declarations (.d.ts files). I think this is essentially for two reasons:

  1. It's tough to maintain types for a package you don't control
  2. The package owners are the best people to understand what the types are

I was on the fence about this for awhile, because I wasn't interested in trying to manually maintain declarations.

Recently, TypeScript introduced support for generation of type declarations from JavaScript sources.

I've used this feature pretty extensively (see report-toolkit) and have had good success with it. The way it works is this:

  1. Use JSDoc-style comment tags, where appropriate, which TS can understand.

    Mocha is already most of the way there, I think. Depending on the extent of the type inference, we may be able to restrict it to a subset the source code (initially, the user-facing, non-programmatic APIs), instead of needing to meticulously work through the entire codebase under lib/, but it's hard to say until we try it.

    Custom types (describing the shape of a plain object or the signature of a callback) are accomplished via @typedef. These are equivalent to TS' type keyword and subject to the same limitations.

    One place where we may get tripped up is that TS is unable to understand prototypal inheritance when not using the class keyword. So, though Test extends Runnable, we won't be able to illustrate that via the declaration--but if we focus on non-programmatic usage, this is moot.

    We need to make sure we're not breaking our API doc generation, though. I don't think this will be much of an issue, as that's mainly focused on the programmatic API. We can revisit the programmatic API at a later point.

  2. Use tsc to generate a .d.ts artifact from the sources we care about. Make it part of the build process; it doubles as a "type linter", because if something is incorrectly typed, the build will fail.
    Publish the resulting file(s) and keep them out of VCS.

Comments? Hate the idea?

@boneskull boneskull added type: feature enhancement proposal qa nice to have enhancement proposal of low priority area: usability concerning user experience or interface area: repository tooling concerning ease of contribution area: integrations related to working with 3rd party software (e.g., babel, typescript) labels Jan 15, 2020
@DanielRosenwasser
Copy link

DanielRosenwasser commented Jan 16, 2020

I'm super happy to see the enthusiasm here! I'd be a bit cautious though because depending on how your types differ, users may see some differences (at least initially) or have dependencies on how the types are declared. I think it's still worth trying to compare/contrast though!

I'd also heavily recommend using checkJs to validate that your types are correct as you do this too - not just using allowJs

@DanielRosenwasser
Copy link

DanielRosenwasser commented Jan 16, 2020

(also we're looking for feedback!)

@weswigham
Copy link

You'd probably wanna try to make sure your API that's generated is compatible with the one exposed by @types/mocha, at least, since once you ship one, it'll override that one; so to make the transition easy, making the type names and structure the same as what's currently documented on DT would be useful.

@boneskull
Copy link
Contributor Author

@weswigham Meaning that if someone has @types/mocha installed, and we start shipping declarations, TS will just ignore the @types/mocha ones?

@weswigham
Copy link

weswigham commented Jan 16, 2020

Yep. A design decision made a long time ago was that a package's own declarations take priority over declarations from the @types namespace - it's more of a fallback than an override.

@juergba
Copy link
Contributor

juergba commented Jan 17, 2020

I had a look at DefinitelyTyped - Mocha, it supports Mocha v5.2 and takes about 3000 lines.
To keep this file up to date seems to be a huge task.

We should start improving our JSDoc for that purpose, but not ship yet any mocha.d.ts file with our package or override @types/mocha.
The past few months it has been difficult enough to publish a release or just to get a review for a PR. So I'm afraid of paralyzing ourself with any additional task we are trying to fulfill.

If we start shipping @types, the quality has to be excellent, otherwise we get flooded by issues we are not able to handle.

@cspotcode was involved in @types/mocha. His point of view would be interesting.

@cspotcode
Copy link
Contributor

Custom types (describing the shape of a plain object or the signature of a callback) are accomplished via @typedef.

We should also be able to sprinkle type-only .ts files into the code for convenience. We can declare types in these files using TS interface / type syntax, then import them elsewhere.

We can use the DefinitelyTyped tests to confirm compatibility. https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/mocha/mocha-tests.ts

Can mocha start shipping its own experimental .d.ts in an opt-in, non-automatic fashion? Consumers would choose to use mocha's bundled declaration if they want, but it would not override @types/mocha. This would be a good way to get feedback and squash bugs.

otherwise we get flooded by issues we are not able to handle.

The Mocha team should have a list of people interested in bugfixing the declarations. Whenever declaration bugs are filed, those people can be pinged and asked to write a PR. I can be on that list to start.

Is mocha's release process automated? If it's straightforward to publish patch releases, this'll be much easier.

@boneskull
Copy link
Contributor Author

How would you ship “opt-in” types?

@boneskull
Copy link
Contributor Author

@juergba

To keep this file up to date seems to be a huge task.

I don’t want to manually maintain a declaration file either.

That’s the idea: it’s generation would be automated from our jsdoc docstrings, of which we have quite a bit already.

but it’s also why this is labeled nice-to-have

@cspotcode
Copy link
Contributor

cspotcode commented Jan 17, 2020

How would you ship “opt-in” types?

I haven't verified that this works, but I have an idea. Maybe the TypeScript team knows if it's do-able.

We could ship a .d.ts that does declare module "mocha" {... but is not referenced in our package.json's "types" field.

If a mocha user wants to opt-in to our bundled types, they have to manually refer to that d.ts file via their tsconfig or a /// <reference path="" /> pragma. If the bundled types are broken, they can remove the pragma and install @types/mocha. Hopefully they do this after filing a helpful bug report. :)

EDIT: to clarify, during this experimental phase, I imagine the types would only be used by adventurous users who know how to configure opt-in and how to help us diagnose any bugs. The goal is to improve the types enough that they can eventually be automatic, and the DefinitelyTyped declarations can be deprecated.

@cspotcode
Copy link
Contributor

cspotcode commented Jan 17, 2020

I switched on checkJs and it's already finding bugs / code smells. It's cool to see this pay off in other ways.

For example, https://github.com/mochajs/mocha/blob/master/lib/cli/run-helpers.js#L85
doesn't match https://nodejs.org/api/fs.html#fs_fs_existssync_path

@cspotcode
Copy link
Contributor

I'm at a stopping point with #4156 for the moment. It builds the declarations and tests them against mocha-tests.ts from DefinitelyTyped. They fail, so there's still work to do. It's a matter of tweaking the JSDoc, fixing typos in mocha's codebase, and identifying bugs like microsoft/TypeScript#36270 if/when they exist.

@cspotcode
Copy link
Contributor

Has anyone had a chance to look at this? Do you have any questions?

@AviVahl
Copy link
Contributor

AviVahl commented Nov 24, 2020

Would you consider accepting a PR converting mocha's own code to (strict) native typescript?

A PR that would typecheck mocha's own code, generate .d.ts from sources, and ensure compatibility with @types/mocha.

@JoshuaKGoldberg
Copy link
Member

Coming back to this a few years later: the general TypeScript community consensus is now that package should only ship their own types if they're able to generate them automatically. As much as I love TypeScript, per #5027 we're not looking to do any major changes like switching the source code language any time soon.

This issue is blocked on us:

  1. Getting much more familiar with Mocha
  2. Filing -> discussing -> approving a separate issue about converting to TypeScript (which is not guaranteed to happen)
  3. Creating & merging a TypeScript PR

@JoshuaKGoldberg JoshuaKGoldberg added core-team issues which must be handled by Mocha's core team status: blocked Waiting for something else to be resolved labels Dec 27, 2023
@JoshuaKGoldberg JoshuaKGoldberg changed the title ship TS type declarations 🚀 Feature: TypeScript .d.ts type declarations Dec 27, 2023
@DanielRosenwasser
Copy link

Note that global declarations are not auto-included for non @types/ packages by default.

As such, if Mocha shipped its own types and you as a consumer wanted all its global declarations included in your project, you'd have to now add something like

{
  "compilerOptions": {
    "types": ["mocha"]
  }
}

That might be considered a best practice, but it's going to break a lot of existing projects relying on automatic types inclusion.

@Uzlopak
Copy link
Member

Uzlopak commented Dec 28, 2023

I personally had the integration of the types of mocha into mocha itself on my agenda, as we do it in fastify. I am not aware of a consensus, that only projects, which generate the typescript types by itself should ship them with the project. @JoshuaKGoldberg Do you have some links regarding this.

@DanielRosenwasser
Thank you for making me aware of this potential issue. Do you have some more links which document this behaviour?

@JoshuaKGoldberg
Copy link
Member

a consensus, that only projects, which generate the typescript types by itself should ship them with the project.

From https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html:

If your types are generated by your source code, publish the types with your source code. Both TypeScript and JavaScript projects can generate types via declaration.

Otherwise, we recommend submitting the types to DefinitelyTyped, which will publish them to the @types organization on npm.

@JoshuaKGoldberg JoshuaKGoldberg added status: in triage a maintainer should (re-)triage (review) this issue and removed status: in triage a maintainer should (re-)triage (review) this issue labels Feb 9, 2024
@JoshuaKGoldberg
Copy link
Member

Just noting for visibility: we're still not ready to take this on. #4228 will have to come first to clean up the internal JSDocs. So if anybody wants to drive this forward, that'd be a good issue to start with!

We also have several other things to get through before we can really give this issue a lot of attention:

  1. Shipping v11: https://github.com/mochajs/mocha/milestone/65
  2. Shipping the new docs site: 📝 Docs: Overhaul website generation #5207 -> docs: add new website using Astro Starlight #5246
  3. Shipping v12: https://github.com/mochajs/mocha/milestone/66

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: integrations related to working with 3rd party software (e.g., babel, typescript) area: repository tooling concerning ease of contribution area: usability concerning user experience or interface core-team issues which must be handled by Mocha's core team nice to have enhancement proposal of low priority status: blocked Waiting for something else to be resolved type: feature enhancement proposal
Projects
Status: Backlog
Development

No branches or pull requests

8 participants