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

[Proof of concept] The ultimate TypeScript setup rework #1975

Closed
wants to merge 27 commits into from

Conversation

aweebit
Copy link
Contributor

@aweebit aweebit commented Aug 17, 2023

Core components of this PR

Why generate .d.ts from JSDoc?

  • one source of truth
  • therefore no inconsistencies between types in .d.ts and JSDoc like currently
  • therefore no chance contributors forget to add a new field / method to .d.ts
  • less work since no manual management of .d.ts is required
  • source maps are generated (source code shown when using Go to Definition, much more useful than just the declaration and developer doesn't have to click around searching for the source file)
  • all fields and methods are exposed as recommended by the TypeScript team
    • Unfortunately, use of methods marked with @package outside Commander is not prevented by TypeScript, but that has little to do with this PR. See #1949 (comment) for details

TODO

  • Manually eliminate inconsistencies between JSDoc and the old .d.ts
  • Add .d.ts files to `.gitignore? (Svelte does that for example)
  • Draw more inspiration from Svelte and SvelteKit

See also

This reverts commit 5c5c972.

Reverted for the cae a different suggestion with a fix for that script
is accepted.
Fixes type errors revealed after enabling useUnknownInCatchVariables
The checkJs TSConfig option will be used instead.

(Most of the directives were misplaced anyway.)
In the test file, an aggregating import (import * as commander) was used
to allegedly import the default export. That is not the same as actually
importing the default export (import commander).

Fixing the import revealed the following error:

TS1192: Module '".../commander.js/typings/index"' has no default export.

The reason is that the default export is not publicly declared because
it is deprecated, and so should not be used.

The test was therefore unnecessary.
The aggregated import (import * as commander) doesn't deliver all
expected functions when "esModuleInterop": true is set in TSConfig,
including when set implicitly because "module" is set to "node16" or
"nodenext", like is now the case in our TSConfig after the last commit.
The new test block covers that.
- Generate typings automatically from JSDoc (npm run build)
- Modularize TSConfig: use separate config files
  - for lib (with typings generation)
  - for Jest tests (with Jest types, checks are opt-in for now)
  - for tsd tests
- Use project references in root TSConfig
  (so that only one tsc run is needed)
- Remove typescript-checkJS script.
  Add typecheck, typecheck-jest and typecheck-all instead
Inspired by SvelteKit (https://github.com/sveltejs/kit)

Motivation:
- the entry points are indeed part of the library source code
- simplifies lint:javascript script and files field in package.json
- elimintates the need for "rootDir": ".." in TSConfig for lib
  (there are no more typings emitted for untracked JS files in the root
  directory, for example config files and files with quick tests/demos)
@aweebit aweebit force-pushed the feature/typescript-workflow branch from c0e79ae to 7bb970b Compare August 17, 2023 08:46
The original, manually written typings are kept in the repo for now
because they still have to be moved to JSDoc which currently differs
from them in a lot of places.
@shadowspawn
Copy link
Collaborator

This is an interesting refactor, but not something I think we would adopt. If we went all-in on typing then I think it is more likely we would switch to TypeScript implementation. We do certainly have parallel JSDoc and TSDoc, but a benefit is tailoring the two interfaces separately. (Although I wouldn't recommend this in a new program!)

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.

2 participants