feat: Add type checking and type definitions #266
Conversation
|
There are merge conflicts here now. |
|
Thanks, fixed. |
There was a problem hiding this comment.
The source code looks great + straightforward to me! Nice to see the existing ad-hoc types generally matching up.
Mainly requesting changes on two things:
- Updating the ESLint config to reflect that this is formal TypeScript-in-JS now
- Using a build system more tailored to compiling TS types
| { src: "src/types.ts", dest: "dist/esm" } | ||
| ] | ||
| }) | ||
| ] |
There was a problem hiding this comment.
Is using Rollup + plugins a settled choice? I've had an easier time with tsup as a more TypeScript-native builder.
There was a problem hiding this comment.
It's what we've been using in all of the type-checked repos so far. Given that it's working for us, I don't think this PR is the place to ponder alternatives. However, it is a bit clunky, so an exploration of another option would be welcome as a separate PR.
| "allowJs": true, | ||
| "checkJs": true, | ||
| "outDir": "dist/esm", | ||
| "target": "ES2022", |
There was a problem hiding this comment.
Strict mode is a strong recommendation for TypeScript projects:
| "target": "ES2022", | |
| "strict": true, | |
| "target": "ES2022", |
If the codebase/team isn't ready for it just yet, there should be a TODO for adding it in.
There was a problem hiding this comment.
I think this is an open question. It might be recommended, but I've found it to be overly cumbersome and I'm not sold that the value is worth the pain. We can follow up on that question later.
| /** | ||
| * @fileoverview Strips typedef aliases from the rolled-up file. This | ||
| * is necessary because the TypeScript compiler throws an error when | ||
| * it encounters a duplicate typedef. |
There was a problem hiding this comment.
Do you have a reference here? My instinct is that one of the following must be true:
- This is a case of the repo's tooling "holding it wrong": which would be a good thing to fix (e.g. by switching from Rollup to something like tsup that's more commonly used with TS builds)
- This is an actual bug in TypeScript that we can link to & I'd want to know more about
- This is a really common thing we'd want to have standardized tooling for (which would surprise me, but you never know!)
There was a problem hiding this comment.
I thought this was intended behavior? Basically, if you have two JSDoc typedefs with the same name, tsc errors out. You can test this for yourself by checking out this branch and removing the dedupe-types step from npm run build and then running it.
There was a problem hiding this comment.
It might be intended behavior on the TypeScript side to error on this case, but it's not intended behavior for the case to be present in the first place. In other words: it's not intended that everybody writing types-in-JS should have to write tools to work around quirks of type checking and transpiling in this way. This kind of bespoke tooling shouldn't be normal or required for any kind of TypeScript projects.
There was a problem hiding this comment.
Gotcha. I guess I assumed it was akin to defining the same variable twice with let or const.
There was a problem hiding this comment.
(commenting here because eslint.config.js isn't changed) Now that files are being generally validated & type checked with TypeScript, the ESLint config should switch from jsdoc.configs["flat/recommended"] to jsdoc.configs["flat/recommended-typescript-flavor"] (or some similar addition of TypeScript). https://github.com/gajus/eslint-plugin-jsdoc/blob/2fbd47c3d2b23a41d87b979daa56a648270d3675/README.md#eslintrc
There was a problem hiding this comment.
That change would need to be made in the main repo, which contains eslint-config-eslint.
|
Merging, as this is blocking my progress on the Markdown language feature. |
This adds TypeScript into the project: