-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Fix type declaration bundle(s) #3038
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
It looks like the .d.ts and .d.cts files that are generated are the same file. Why do we need both? |
Because the file extension describes something very important. A |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this! 💯 I am not a typescript expert so it is great to have the community watching out for things like this.
5b81d9a
to
03dccd8
Compare
@@ -45,6 +50,7 @@ | |||
"html" | |||
], | |||
"devDependencies": { | |||
"@arethetypeswrong/cli": "^0.12.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this dependency used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here (line 95): https://github.com/markedjs/marked/pull/3038/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R95
It’s optional, but I recommend it any time types and JS are generated by separate tools, as opposed to a single tsc
run. As evidenced by the issue this PR fixes, it is quite easy to make JS that disagrees with the types when using a tool like Rollup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, thats the attw
bin 👍
Oddly enough, @arethetypeswrong/cli
depends on marked
😆
https://www.npmjs.com/package/@arethetypeswrong/cli?activeTab=dependencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That’s how I noticed this issue! 😄
package.json
Outdated
@@ -94,7 +101,7 @@ | |||
"build:reset": "git checkout upstream/master lib/marked.cjs lib/marked.umd.js lib/marked.esm.js marked.min.js", | |||
"build": "npm run rollup && npm run build:types && npm run build:man", | |||
"build:docs": "npm run build && node docs/build.js", | |||
"build:types": "tsc --project tsconfig-types.json", | |||
"build:types": "dts-bundle-generator --project tsconfig.json -o lib/marked.d.ts src/marked.ts && dts-bundle-generator --project tsconfig.json -o lib/marked.d.cts src/marked.ts", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will dts-bundle-generator
fail when tsc
would fail? Usually these bundlers need something like tsc --noEmit
first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I didn’t notice that nothing else here is running tsc
for validation. Do you want me to add that to build:types
or another script?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya I think we would want build:types
to fail if tsc
fails
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do use tsc
in test:types. I'm not sure if that is good enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍 (tsc
with no arguments uses the file named tsconfig.json
by default which is what you want—it already has noEmit
and it will reflect exactly the errors you see in-editor)
21e92c7
to
e47ef5f
Compare
## [9.1.4](v9.1.3...v9.1.4) (2023-10-31) ### Bug Fixes * Fix type declaration bundle(s) ([#3038](#3038)) ([a7b402c](a7b402c))
Marked version:
9.1.2
Markdown flavor: n/a
Description
Related: #2997
👋 Hi, I’m the author of https://arethetypeswrong.github.io. I noticed this problem because
marked
is a dependency of the project, but coincidentally, the purpose of the project is to detect and diagnose issues exactly like this.marked
ships both an ESM and CJS bundle, but attempts to share types generated withtsc --outFile
between both. This does not work for a few reasons. The declaration files made with--outFile
declare ambient modules that don’t exist:allows any user to import from
"rules"
once they’ve imported from"marked"
:Additionally, the
.d.ts
file is recognized as typing an ES module, which causes--moduleResolution nodenext
users to not be able to importmarked
from CommonJS files, as in #2997 (which doesn’t seem to be related to@types/marked
to me).You can see details about this problem, with a linked explainer, at https://arethetypeswrong.github.io/?p=marked%409.1.2
Expectation
Types work in TypeScript configurations reflecting runtimes where
marked
worksResult
Types do not work in CommonJS files in
--moduleResolution nodenext
, even thoughmarked
itself works. Additionally, types declare modules that do not exist at runtime.What was attempted
This PR generates .d.ts bundles for each module format with a tool that was designed to do this: https://github.com/timocov/dts-bundle-generator
It also validates the result with @arethetypeswrong/cli to prevent future regressions of a similar variety.
Contributor
Committer
In most cases, this should be a different person than the contributor.