Closed FunkMonkey closed 1 year ago
wooorm
commented
2 years ago dev-dep wouldn’t work as it’s an actual dependents.
The goal of @types/web is to exactly not require the DOM environment types.
It’s a bit alpha. So if you have a better solution, feel free to send a PR to improve things.
FunkMonkey
commented
2 years ago 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 @types/web is the way to do so.
I agree that it is debatable, whether @types/whatever should be in dependencies instead of devDependencies at all, considering @types are only necessary at dev time and have no runtime effect whatsoever.
Considering that using @types/web as a dependency is basically forcing the users of your library to switch from lib: ["dom"] in tsconfig to @types/web themselves, I would propose to "downgrade" it to a devDependency (unleass you have good reasons to want to lock your DOM types to a specific @types/web version...)
What do you think?
wooorm
commented
2 years ago I get that you want to be in control of which version of the DOM types are used and that
@types/webis the way to do so.
That is not really the reason. The reason is I don’t want end users to have to enable 'dom', because this project also works well in other environments that are not browsers. For example, with jsdom.
I agree that it is debatable, whether
@types/whatevershould be independenciesinstead ofdevDependenciesat all, considering @types are only necessary at dev time and have no runtime effect whatsoever.
The difference is as follows, given that you develop app A, which uses my package B, and my package uses types from package C. If B depends on C as a dev-dependency, you get TypeScript errors about the package not being there, and you have to install it in your project. If B depends on C as a dependency, you don’t get errors. You don’t need to install my dependencies.
Similarly, if I want 'dom' in my package B, you must enable 'dom'. I don’t want that.
Considering that using
@types/webas adependencyis basically forcing the users of your library to switch fromlib: ["dom"]in tsconfig to@types/webthemselves, I would propose to "downgrade" it to adevDependency(unleass you have good reasons to want to lock your DOM types to a specific@types/webversion...)
This should not happen. You are also the first to notice a problem with this, even though I use @types/web in other (frequently used) packages too.
If it happens, that is because:
a) you have configured TypeScript wrong (e.g., perhaps you have a very old one),
b) it is a bug in @types/web, unlikely, because they publish the same types as 'dom',
c) it is a bug here, perhaps because @types/web is out of date. That is likely due to them releasing patch releases, it would be easier if they followed semver, at least with a minor version, perhaps you can raise an issue with them to do that, if you can assert that this is the case.
FunkMonkey
commented
1 year ago @wooorm I am sorry for not responding again! I finally figured out what the problem is: it is how @types/web is used in your repository.
The docs say to install @types/web via yarn add @typescript/lib-dom@npm:@types/web. This will only result in this being added to the package.json:
"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 @types/web as a dependency itself. If so, it would result in having two packages (@types/web and @typescript/lib-dom) both with the contents of @types/web. This again leads to duplicate identifier errors.
Am I correct with my analysis this time? If so, would you accept a PR regarding this change?
Thanks a lot!
wooorm
commented
1 year ago I don’t know if that is correct!
I think what they mention there, and what you are doing with remapping @typescript/lib-dom, is to change the entire DOM typings.
That’s exactly what we don’t want to do? 🤔 I don’t want to require DOM in lib
FunkMonkey
commented
1 year ago Thank you for your quick response!
I think what they mention there, and what you are doing with remapping @typescript/lib-dom, is to change the entire DOM typings.
As far as I understand it, this type of remapping is the exact point of @types/web ;)
The npm page says:
@types/web is also included inside TypeScript, available as dom in the lib section and included in projects by default. By using @types/web you can lock your the web APIs used in your projects, easing the process of updating TypeScript and offering more control in your environment.
At least from the docs, it thus seems that @types/web should not be a used as a direct dependency. People who don't specifically use it, get some version anyway in the form of typescript/lib/lib.dom.d.ts (see TypeScript-DOM-Lib-Generator). People who use @types/web as described on its npm page, will have it downloaded in @typescript/lib-dom (basically redirected, the package.json of @typescript/lib-dom even says "name": "@types/web"). So @types/web is always already present in one form or the other.
Before you said:
The reason is I don’t want end users to have to enable 'dom', because this project also works well in other environments that are not browsers. For example, with jsdom.
I totally understand that! I assume though, that browser-based environments do use dom anyway (and thus either @typescript/lib-dom, which equals @types/web; or typescript/lib/lib.dom.d.ts) . And jsdom users will use the jsdom types. So I guess there is no need to have @types/web as a dependency at all.
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"]
}node_modules/typescript/lib/lib.dom.d.ts:25653:5 - error TS2374: Duplicate index signature for type 'number'.
25653 [index: number]: Window;
~~~~~~~~~~~~~~~~~~~~~~~~
Found 100 errors in 2 files.
Errors Files
55 node_modules/@types/web/index.d.ts:7
45 node_modules/typescript/lib/lib.dom.d.ts:23Interestingly Typescript is smart enough to ignore lib.dom.d.ts when @typescript/lib-dom exists (which I guess is hardcoded as part of the lib-replacement feature). This is the reason that redirecting @types/web to @typescript/lib-dom via yarn add @typescript/lib-dom@npm:@types/web works - at least as long as @types/web is not installed by something else (e.g. rehype-mathjax).
Long story short: Whenever DOM is enabled, @types/web will lead to conflicting types.
Hope this helps! I guess the magic of how Typescript uses DOM types makes this all kinda complicated :)
remcohaszing
commented
1 year ago @FunkMonkey is correct here. The intended usage of @types/web is so users can swap builtin dom libs using the dependency:
{
"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 https://github.com/remarkjs/remark-math/issues/75#issuecomment-1633803990.
The technically correct way for a library to depend on the DOM types, is to reference the dom lib:
/// <reference lib="dom" />Often this is just omitted though.
github-actions[bot]
commented
1 year ago Hi team! I don’t know what’s up as there’s no phase label. Please add one so I know where it’s at.
Thanks, — bb
wooorm
commented
1 year ago @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?
remcohaszing
commented
1 year ago mathjax-full indeed requires the DOM types. As a result, rehype-mathjax would need the DOM types to build it locally. The mathjax-full types are broken though, so skipLibCheck is enabled, meaning none of this matters.
More importantly, when using rehype-mathjax, it does not need the DOM types.
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 @types/web are included
$ 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.tsHowever, many projects do need the dom types and don’t define "types" to exclude @types/web, so when we change tsconfig.json:
// tsconfig.json
{}And when we list the files again, we see a lot of errors indicating conflict between @types/web and the builtin dom types
$ 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:23So really having @types/web as a dependency causes a lot of conflicts and rehype-mathjax doesn’t even use it.
wooorm
commented
1 year ago I’m guessing something changed over the years because I am pretty sure stuff failed before
FunkMonkey
commented
1 year ago 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