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

Add missing types dependency #3

Merged

Conversation

Methuselah96
Copy link
Contributor

@Methuselah96 Methuselah96 commented Sep 12, 2022

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

This PR moves @types/mdast back to dependencies since the declaration files still reference them. This was initially done in #2, but then moved to a devDependency in c7c9178. @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 github-actions bot added the 👋 phase/new Post is being triaged automatically label Sep 12, 2022
@Methuselah96 Methuselah96 changed the title Move @types/mdast to dependencies since declaration files reference them Move @types/mdast to dependencies Sep 12, 2022
@github-actions github-actions bot added 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Sep 12, 2022
@Methuselah96 Methuselah96 changed the title Move @types/mdast to dependencies Add missing dependency for types Sep 12, 2022
@wooorm wooorm changed the title Add missing dependency for types Add missing types dependency Sep 12, 2022
@wooorm wooorm added ☂️ area/types This affects typings 💪 phase/solved Post is done labels Sep 12, 2022
@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Sep 12, 2022
@wooorm wooorm merged commit 18b251c into syntax-tree:main Sep 12, 2022
@github-actions
Copy link

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
Copy link
Member

wooorm commented Sep 12, 2022

Which declaration file references them?

@wooorm
Copy link
Member

wooorm commented Sep 12, 2022

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 Methuselah96 deleted the move-types-mdast-back-to-dependencies branch September 12, 2022 13:50
@Methuselah96
Copy link
Contributor Author

Methuselah96 commented Sep 12, 2022

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
Copy link
Member

wooorm commented Sep 13, 2022

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: microsoft/TypeScript#38111.

@Methuselah96
Copy link
Contributor Author

Methuselah96 commented Sep 13, 2022

Out of curiosity, have you all ever considered actually just using TypeSript instead of JSDoc? It seems possible that #3, remarkjs/remark#1039, and remarkjs/react-markdown#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
Copy link
Member

wooorm commented Sep 13, 2022

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
Copy link
Contributor Author

Methuselah96 commented Sep 13, 2022

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
Copy link
Member

wooorm commented Sep 13, 2022

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:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
☂️ area/types This affects typings 💪 phase/solved Post is done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants