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

Generate type declarations from JSDoc #3830

Merged
merged 1 commit into from
Sep 25, 2024

Conversation

voxpelli
Copy link
Contributor

@voxpelli voxpelli commented Sep 23, 2024

Building upon #3732 to fix #3797 – this mimics the flow that I use in multiple places to generate type declarations from JSDoc comments – eg. in as neostandard (which uses this plugin and which caused me to look into generating types here) and others that use my types-in-js oriented tsconfig.

This PR consists of three commits:

  1. The type generation itself – one npm script for generating them and one for cleaning them up by deleting them (reason for cleaning is not only to keep it clean, its also because TypeScript always picks a .d.ts file over a .js file of the same name, so when the types are generated the JSDoc comments are essentially ignored until one remove the generated .d.ts files)
  2. A GitHub Actions workflow that tests the generated types against multiple TypeScript versions and TypeScript library configurations using a dummy module in test-published-types/ – to ensure its compatible with all of them without the need to use skipLibCheck: true (which one should not set, see eg. Remove skipLibCheck voxpelli/tsconfig#1). Since ESLint >=9.10 has built in types it uses that for some compatibility testing as well. Some extra dependencies was needed for that as well, see fix: add @eslint/core, @types/estree, & @types/json-schema deps eslint/eslint#18938 And sadly those types does not support TS 3.9, so testing matrix start at 4.0. The workflow is inspired by my personal one which I use in eg. pony-cause
  3. Some tweaks to the JSDoc types, to ensure the generated types and the added workflow all passes

I have tried to adapt these additions to the style and approach of this project. I did add one // eslint-disable-next-line valid-jsdoc in place, I hope that's okay, else we'll work something out.

Fixes #3797

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Interesting!

.eslintignore Outdated Show resolved Hide resolved
.github/workflows/type-check.yml Show resolved Hide resolved
.github/workflows/type-check.yml Outdated Show resolved Hide resolved
.github/workflows/type-check.yml Outdated Show resolved Hide resolved
lib/rules/jsx-no-literals.js Show resolved Hide resolved
lib/util/makeNoMethodSetStateRule.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@ljharb
Copy link
Member

ljharb commented Sep 23, 2024

(re valid-jsdoc, yes, it's fine, i'm going to be ditching that rule entirely in the next semver-major and only using tsdoc)

Copy link

codecov bot commented Sep 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.68%. Comparing base (59ef14c) to head (e665b69).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3830      +/-   ##
==========================================
- Coverage   97.73%   97.68%   -0.05%     
==========================================
  Files         133      136       +3     
  Lines        9917     9930      +13     
  Branches     3678     3679       +1     
==========================================
+ Hits         9692     9700       +8     
- Misses        225      230       +5     
Flag Coverage Δ
97.68% <100.00%> (?)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@voxpelli
Copy link
Contributor Author

@ljharb Is the "Automatic Rebase / _ / Automatic Rebase" failure something I should adress by rebasing this PR myself and force pushing? Or by eg. merging master into this branch? Or something you'll deal with on merge?

@ljharb
Copy link
Member

ljharb commented Sep 24, 2024

@voxpelli i'll deal with it before merging, you can ignore it - it fails sometimes despite a PR being rebaseable and maintainers having edit permissions ¯\_(ツ)_/¯

@ljharb
Copy link
Member

ljharb commented Sep 24, 2024

Is there a reason d.ts generation is preferred over manually authoring d.ts files, and tsdoc-importing definitions in comments?

@ljharb ljharb force-pushed the generate-type-declarations branch from 3df37cf to 7f3ac1b Compare September 24, 2024 16:51
@voxpelli
Copy link
Contributor Author

Is there a reason d.ts generation is preferred over manually authoring d.ts files, and tsdoc-importing definitions in comments?

Often when people author d.ta files that don't reimport them – so they are essentially creating a parallel universe that may or may not mirror the actual code that they are shopping.

Automating it means that all of the generated types are validated against and based on the actual implementation – that way one can be sure that it never becomes out of sync

I sometimes to author .d.ts files, but mostly for helpers that becomes much less verbose to write in TS syntax than in JSDoc, so I write them there and then import them, and that works great as well I think

I sometimes write the index.d.ts by hand as well, to ensure that I have full control of which top level types that are exported, and can easily export additional types that may not be exported automatically from index.js otherwise, but which may be needed to be referenced in callbacks or configs or such

@voxpelli
Copy link
Contributor Author

Also:

When generating it automatically from JS one can easily add the declaration source maps that I enabled in this PR, and that makes it so that when one wants to find the definition of a function in eg VS Code it will not jump to the type file, it will jump to the code file, which is nice.

We actually discussed it here today: https://x.com/voxpelli/status/1838625086494118053?s=46&t=1mQKe1AKaQ-2YwRjxDfrOg

@ljharb
Copy link
Member

ljharb commented Sep 25, 2024

ah interesting, i hadn't thought about that. all my packages that ship types so far use hand-written d.ts files that are imported into the JS files, ensuring they're correct, but i wasn't aware of the source map/declaration map nuance.

@ljharb ljharb merged commit 7f3ac1b into jsx-eslint:master Sep 25, 2024
376 checks passed
renovate bot added a commit to mmkal/eslint-plugin-mmkal that referenced this pull request Sep 27, 2024
##### [v7.37.0](https://github.com/jsx-eslint/eslint-plugin-react/blob/HEAD/CHANGELOG.md#7370---20240926)

##### Added

-   add type generation ([#3830][] [@voxpelli](https://github.com/voxpelli))
-   \[`no-unescaped-entities`]: add suggestions ([#3831][] [@StyleShit](https://github.com/StyleShit))
-   \[`forbid-component-props`]: add `allowedForPatterns`/`disallowedForPatterns` options ([#3805][] [@Efimenko](https://github.com/Efimenko))
-   \[`no-unstable-nested-components`]: add `propNamePattern` to support custom render prop naming conventions ([#3826][] [@danreeves](https://github.com/danreeves))

##### Changed

-   \[readme] flat config example for react 17+ ([#3824][] [@GabenGar](https://github.com/GabenGar))

[7.36.2]: jsx-eslint/eslint-plugin-react@v7.36.1...v7.36.2

[#3831]: jsx-eslint/eslint-plugin-react#3831

[#3830]: jsx-eslint/eslint-plugin-react#3830

[#3826]: jsx-eslint/eslint-plugin-react#3826

[#3824]: jsx-eslint/eslint-plugin-react#3824

[#3805]: jsx-eslint/eslint-plugin-react#3805
@voxpelli voxpelli deleted the generate-type-declarations branch December 9, 2024 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Add TypeScript types
2 participants