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

Use JSDoc to check and generate TypeScript typings #5

Merged
merged 2 commits into from
Aug 18, 2021

Conversation

ChristianMurphy
Copy link
Member

@ChristianMurphy ChristianMurphy commented Apr 6, 2020

Rendered version: https://github.com/unifiedjs/rfcs/blob/963c1c20de118a2e9e60e249e56a5f18c573c8fa/text/0000-generate-typings-from-jsdoc.md

This would also be made easier by #4
Blog post by another project (and a remark adopter) who have also taken this approach https://dev.to/open-wc/generating-typescript-definition-files-from-javascript-5bp2
Example of what this would look like in unified vfile/vfile-message#8

@ChristianMurphy ChristianMurphy added 🕸️ area/tests This affects tests 🏗 area/tools This affects tooling ☂️ area/types This affects typings 🗳 blocked/vote This cannot progress before voting is complete 🙉 open/needs-info This needs some more info 💬 type/discussion This is a request for comments labels Apr 6, 2020
@ChristianMurphy ChristianMurphy requested review from a team April 6, 2020 01:36
@ChristianMurphy
Copy link
Member Author

/cc @Rokt33r @JounQin

@wooorm
Copy link
Member

wooorm commented Apr 6, 2020

I think this is a great idea! unified stuff used to have JSDocs way back when!

@wooorm
Copy link
Member

wooorm commented Apr 6, 2020

Another drawback is that the code side increases a lot. The code I just linked had 49 lines of code, the file with comments is 139 lines. That’s almost tripling the size.

If we pull index.d.ts and # API in readme.md from the code, that may be worth it though.

Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

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

This makes sense! The barrier for me when adding types to an existing package for the first time was figuring out imports, name spaces, and other specific TypeScript things I did not encounter before. This switches to a more 'natural' type specification and leaves the details to the compiler.

However, adding comments to test files to also test types feels a bit weird to me. I’m not sure how extensive the generated test.d.ts file is out-of-the-box and what our standards are for type tests. But I feel that part might make more sense explicitly in its own file. The downside is that it brings in extra config files, though we already have those in the existing approach.

@ChristianMurphy
Copy link
Member Author

ChristianMurphy commented Apr 6, 2020

Another drawback is that the code side increases a lot. The code I just linked had 49 lines of code, the file with comments is 139 lines. That’s almost tripling the size.

Good point, updating RFC to include this.

If we pull index.d.ts and # API in readme.md from the code, that may be worth it though.

Another RFC for this is in the works.

However, adding comments to test files to also test types feels a bit weird to me.

Good point, I may add a dedicated section to the RFC for this.
The thinking here is that the annotations in the tests would not be a part of the public API or the typings.
This would be suppressed through gitignore and JSDoc annotations.
It would allow for the existing test suite to check both how the code runs, and validate that the typings work.

But I feel that part might make more sense explicitly in its own file. The downside is that it brings in extra config files, though we already have those in the existing approach.

I see your point, and I'm open to the idea.
The reason I didn't go that direction initially are duplication and missed cases:

  • duplication type tests need to follow a similar coverage path to unit tests, having both would lead to some overlap
  • missed cases when type tests and unit tests are used together they can articulate edge cases that may be difficult to capture otherwise, For example an API which can return number may return NaN in some circumstances, which is perfectly valid from a static typing perspective, and would only be seen in the unit tests. Having the unit tests also be the type tests gives a single source of truth for seeing what those edge cases may be.

Another alternative would be to make test file a .ts file and use ts-node to run the test suite.

Co-authored-by: Titus Wormer <tituswormer@gmail.com>
@Rokt33r
Copy link
Member

Rokt33r commented Apr 10, 2020

Hmm.. I think using JSDoc would work for simple projects. But for unified ecosystem, I'm a bit worrying.

I've checked how interfaces works in JSDoc but I couldn't find proper ways to importing and exporting those interfaces. I think this is quite important for unified because most of packages of unified are using Node, VFile and Plugin interfaces or type alias.

Also we might lose some keywords like any and unknown.

How can we tackle those problems if we adopt JSDoc?

@ChristianMurphy
Copy link
Member Author

Thanks for the feedback @Rokt33r! 🙇

I've checked how interfaces works in JSDoc but I couldn't find proper ways to importing and exporting those interfaces.

This is a concern I initially shared.
And is part of why I opened vfile/vfile-message#8 before opening this RFC.
An example of importing and exporting interfaces is here: https://github.com/vfile/vfile-message/blob/4fd342b923abe68c8ce0371cf82c5ce807f52fb4/index.d.ts#L2-L12

Documentation for these is located at https://www.typescriptlang.org/docs/handbook/type-checking-javascript-files.html#import-types and https://www.typescriptlang.org/docs/handbook/type-checking-javascript-files.html#typedef-callback-and-param

Also we might lose some keywords like any and unknown.

While I would generally prefer to stay away from both of these types.
Both any and unknown are supported https://www.typescriptlang.org/docs/handbook/type-checking-javascript-files.html#type

@Rokt33r
Copy link
Member

Rokt33r commented Apr 10, 2020

Thanks for clarification. I've read the article from typescript just now. It looks good to me. I also want to try to adopt it. 👍

@wooorm
Copy link
Member

wooorm commented Feb 15, 2021

for anyone reading along, @ChristianMurphy and I have recently tried this in xdm, works pretty well, although indeed a bit verbose: https://github.com/wooorm/xdm/blob/964eb45052e7fe130788540521b8ec81a1138fea/lib/core.js#L13

@JounQin
Copy link
Member

JounQin commented Feb 20, 2021

Why not use .ts directly. 😂

@wooorm
Copy link
Member

wooorm commented Feb 21, 2021

I think these are my main three reasons on why I prefer jsdoc ts over .ts:

  • TS is a understood by less people than JS
  • It adds an extra compile step
  • I think it’s more likely that TC39 standardizes jsdoc, or a different type system, instead of how .ts files work currently, meaning that similar to CoffeeScript, in a couple years we’ll all be rewriting things back into JS

@ChristianMurphy
Copy link
Member Author

  • I think it’s more likely that TC39 standardizes jsdoc, or a different type system, instead of how .ts files work currently,

It is possible.
But not the most likely outcome, more likely is something like pep-0526 did for Python where annotations are standardized, but types themselves may not be, Brendan Eich one of the original creators of JS spoke about this recently in an interview.
Which would also remove the second concern

It adds an extra compile step


In the meantime, JSDoc based annotations allow authors and maintainers that prefer vanilla javascript to continue doing so, and also generate typings which keep in sync with the code.
This also addresses one of the issues with handwritten dts files, namely that the types can get out of sync with the implementation with little to no warning. While with JSDoc based annotations, the type checker can flag cases where things are out of sync.

@ChristianMurphy
Copy link
Member Author

Another possible value point for JSDoc, it could be used to generate the readme documentation, for example something like: https://github.com/jsdoc2md/jsdoc-to-markdown

@ChristianMurphy
Copy link
Member Author

JSDoc based types has been rolled out to most of unified as part of unifiedjs/unified#121
Would it make sense to merge this? or close it?

@wooorm wooorm merged commit db93813 into unifiedjs:main Aug 18, 2021
@wooorm wooorm added 💪 phase/solved Post is done and removed 🗳 blocked/vote This cannot progress before voting is complete 🙉 open/needs-info This needs some more info labels Aug 18, 2021
@ChristianMurphy ChristianMurphy deleted the docs/rfc-typescript-with-jsdoc branch August 22, 2021 13:45
@wooorm wooorm mentioned this pull request Oct 27, 2021
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🕸️ area/tests This affects tests 🏗 area/tools This affects tooling ☂️ area/types This affects typings 💪 phase/solved Post is done 💬 type/discussion This is a request for comments
Development

Successfully merging this pull request may close these issues.

5 participants