-
-
Notifications
You must be signed in to change notification settings - Fork 56
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
rehype-mathjax should not have a dependency on @types/web #75
Comments
dev-dep wouldn’t work as it’s an actual dependents. The goal of |
Hi @wooorm, Thank you for the quick reply! I get that you want to be in control of which version of the DOM types are used and that I agree that it is debatable, whether Considering that using What do you think? |
That is not really the reason. The reason is I don’t want end users to have to enable
The difference is as follows, given that you develop app A, which uses my package B, and my package uses types from package C. Similarly, if I want
This should not happen. You are also the first to notice a problem with this, even though I use |
@wooorm I am sorry for not responding again! I finally figured out what the problem is: it is how The docs say to install "dependencies": {
"@typescript/lib-dom": "npm:@types/web", // this can also be tied with a specific version
"typescript": "5.1.6"
} It seems it is not necessary to have Am I correct with my analysis this time? If so, would you accept a PR regarding this change? Thanks a lot! |
I don’t know if that is correct! |
Thank you for your quick response!
As far as I understand it, this type of remapping is the exact point of The npm page says:
At least from the docs, it thus seems that Before you said:
I totally understand that! I assume though, that browser-based environments do use The problem is: apparently this even leads to problems, for everyone who does use DOM. E.g this simple case leads to conflicts: {
"name": "test",
"type": "module",
"version": "0.0.1",
"main": "build/index.js",
"scripts": {
"build": "tsc"
},
"dependencies": {},
"devDependencies": {
"@types/web": "^0.0.104",
"typescript": "5.1.6"
}
} {
"compilerOptions": {
"lib": ["ES2021", "DOM"],
"outDir": "build",
"rootDir": "./src",
"baseUrl": "./",
"rootDir": "./src",
"sourceMap": true,
"target": "ES2021"
},
"include": ["./src/**/*.ts"]
}
Interestingly Typescript is smart enough to ignore Long story short: Whenever DOM is enabled, Hope this helps! I guess the magic of how Typescript uses DOM types makes this all kinda complicated :) |
@FunkMonkey is correct here. The intended usage of {
"devDependencies": {
"@typescript/lib-dom": "npm:@types/web"
}
} Libraries should certainly not include this, as it results in clashes for TypeScript globals, as was demonstrated in #75 (comment). The technically correct way for a library to depend on the DOM types, is to reference the /// <reference lib="dom" /> Often this is just omitted though. |
This comment has been minimized.
This comment has been minimized.
@remcohaszing I believe this project does not need the DOM AFAIK? MathJax apparently does, but it’s not needed for us. The problem is: I don’t want users to enable DOM types? How to fix that? |
More importantly, when using Given the following files: // tsconfig.json
{
"compilerOptions": {
// This explicitly excludes `dom`
"lib": ["es5"],
// This explicitly excludes `@types/web`
"types": []
}
} // index.ts
import 'rehype-mathjax' The following shows that neither the DOM types, nor $ tsc --listFiles
node_modules/typescript/lib/lib.es5.d.ts
node_modules/typescript/lib/lib.decorators.d.ts
node_modules/typescript/lib/lib.decorators.legacy.d.ts
node_modules/@types/unist/index.d.ts
node_modules/vfile-message/lib/index.d.ts
node_modules/vfile-message/index.d.ts
node_modules/vfile/lib/minurl.shared.d.ts
node_modules/vfile/lib/index.d.ts
node_modules/vfile/index.d.ts
node_modules/unified/index.d.ts
node_modules/@types/hast/index.d.ts
node_modules/rehype-mathjax/lib/create-plugin.d.ts
node_modules/rehype-mathjax/svg.d.ts
node_modules/rehype-mathjax/index.d.ts
index.ts However, many projects do need the // tsconfig.json
{} And when we list the files again, we see a lot of errors indicating conflict between $ tsc --listFiles
# Lots of errors here truncated for brevity.
node_modules/typescript/lib/lib.d.ts
node_modules/typescript/lib/lib.es5.d.ts
node_modules/typescript/lib/lib.dom.d.ts
node_modules/typescript/lib/lib.webworker.importscripts.d.ts
node_modules/typescript/lib/lib.scripthost.d.ts
node_modules/typescript/lib/lib.decorators.d.ts
node_modules/typescript/lib/lib.decorators.legacy.d.ts
node_modules/@types/unist/index.d.ts
node_modules/vfile-message/lib/index.d.ts
node_modules/vfile-message/index.d.ts
node_modules/vfile/lib/minurl.shared.d.ts
node_modules/vfile/lib/index.d.ts
node_modules/vfile/index.d.ts
node_modules/unified/index.d.ts
node_modules/@types/hast/index.d.ts
node_modules/rehype-mathjax/lib/create-plugin.d.ts
node_modules/rehype-mathjax/svg.d.ts
node_modules/rehype-mathjax/index.d.ts
index.ts
node_modules/@types/mathjax/index.d.ts
node_modules/@types/web/iterable.d.ts
node_modules/@types/web/index.d.ts
Found 1268 errors in 3 files.
Errors Files
1011 node_modules/@types/web/index.d.ts:7
204 node_modules/@types/web/iterable.d.ts:6
53 node_modules/typescript/lib/lib.dom.d.ts:23 So really having |
I’m guessing something changed over the years because I am pretty sure stuff failed before |
released, thanks y’all! |
Thank you @wooorm and @remcohaszing! :) |
Initial checklist
Affected packages and versions
4.0.2
Link to runnable example
No response
Steps to reproduce
Install reyhpe-mathjax via a package manager
Expected behavior
The typescript project referencing rehype-mathjax should not throw any new errors.
Actual behavior
Typescript (e.g. in VSCode) gives the following errors for@types/web and @typescript/lib-dom, because we basically have double definitions.
Should rehype-mathjax not only have @types/web as a devDependency or peerDependency so it does not interfere with the typescript setup of the project that uses it?
Why is the specific version of @types/web here required anyway?
Thanks a lot for your support!
Runtime
Node v16
Package manager
yarn 1
OS
Windows
Build and bundle tools
Vite
The text was updated successfully, but these errors were encountered: