search

syntax-tree / mdast-util-gfm-table

mdast extension to parse and serialize GFM tables
https://unifiedjs.com
MIT License
12 stars 7 forks source link

Add missing types dependency #3

Closed Methuselah96 closed 2 years ago

Methuselah96 commented 2 years ago

Initial checklist

Description of changes

This PR moves @types/mdast back to dependencies since the declaration files still reference them. This was initially done in https://github.com/syntax-tree/mdast-util-gfm-table/pull/2, but then moved to a devDependency in https://github.com/syntax-tree/mdast-util-gfm-table/commit/c7c917851937251be18e5ddb547b49625fdad408. @types/mdast still needs to be listed as a dependency since @types/mdast is still referenced in the generated lib/index.d.ts file.

github-actions[bot] commented 2 years ago

Hi! This was closed. Team: If this was merged, please describe when this is likely to be released. Otherwise, please add one of the no/* labels.

wooorm commented 2 years ago

Which declaration file references them?

wooorm commented 2 years ago

I was under the impressions that users would not get warnings from internal types, used to check things internally? Is this because you have skipLibCheck on? I believe we have this in tons of places?

Methuselah96 commented 2 years ago

Which declaration file references them?

lib/index.d.ts

I was under the impressions that users would not get warnings from internal types, used to check things internally? Is this because you have skipLibCheck on? I believe we have this in tons of places?

Yes, one would not get these errors if skipLibCheck were turned on. However, my understanding is that the skipLibCheck option exists to save time for the type-checker and is not meant to be used as a way to ignore errors in the declaration files that are being consumed by the user. There is no way to distinguish between the "acceptable errors" and the "errors that could cause problems for the user" and so my only option as a user is to skipLibCheck turned off to catch any errors that could cause problems.

As for "internal" vs "external" types, the way the types are currently defined, they are "external" in the sense that they're included in the declaration files and therefore any consumer of this package has to be able to parse them/type-check them. For example this JSDoc code:

/**
 * @typedef {import('mdast').AlignType} AlignType
 */

gets converted to this TypeScript declaration:

export type AlignType = import('mdast').AlignType;

The only way to avoid this using JSDoc that I know of would be not to use @typedef for the dependency that you don't want to show up in the declaration file (and instead inline those type references).

wooorm commented 2 years ago

Yes, you are right that this is about typedefs. It would be better if it had explicit imports and exports instead of only exports. But even then, TS is generating paths that might be different from what npm/yarn/pnpm use: https://github.com/microsoft/TypeScript/issues/38111.

Methuselah96 commented 2 years ago

Out of curiosity, have you all ever considered actually just using TypeSript instead of JSDoc? It seems possible that https://github.com/syntax-tree/mdast-util-gfm-table/pull/3, https://github.com/remarkjs/remark/issues/1039, and https://github.com/remarkjs/react-markdown/pull/676 could have been avoided if these packages were actually using TypeScript. My impression is that a lot of these are just rough spots with TypeScript since they don't focus their effort on making JSDoc typedefs work well and that they wouldn't be an issue with normal TypeScript imports.

wooorm commented 2 years ago

Sure, everything’s been discussed at length. You should be able to find threads on this.

I have pretty strong opinions about this, particularly for (Node-style) open source packages that live for 10, 20+ years. Other maintainers (e.g rich harris) feel similar about preferring JSDoc.

The problem is that TypeScript has a bunch of bugs and that it doesn’t adhere to SemVer. That’s on TS. Why would I now rewrite a perfectly working project into an unstable, buggy, compile-to-JavaScript language? Worst-case the code doesn’t compile anymore.

Types can be useful. TypeScript has problems: a) bunch of bugs, b) it’s a proprietary compile-to-javascript language. It’ll go away.

Methuselah96 commented 2 years ago

Yeah, that's along the lines of what I assumed. To be fair though, you're still doing the type-checking which is the part that is unstable/buggy/doesn't adhere to SemVer. The actual converting to JavaScript is just dropping the type annotations (assuming you're not using enums) which is fairly trivial, so the "lock-in" doesn't seem that bad.

(Feel free to disengage, I'm not trying to draw out a conversation if you don't feel it's worth your time, I'm mostly just curious about your position.)

wooorm commented 2 years ago

you're still doing the type-checking

Indeed. But now that type checking of TS is pulled away from using a custom language. I don’t have a problem with type checking.

just getting dropping the type annotations

I see the TS language as a bit more complex that that. But, once types-as-comments exist, and are blessed by the language, I’d likely use ’em!

Some more reading: