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

The current types include globals that don't exist in Node #285

Open
karfau opened this issue Aug 20, 2021 · 9 comments
Open

The current types include globals that don't exist in Node #285

karfau opened this issue Aug 20, 2021 · 9 comments
Labels
bug Something isn't working help-wanted External contributions welcome invalid This doesn't seem right types Anything regarding Typescript

Comments

@karfau
Copy link
Member

karfau commented Aug 20, 2021

This is a "copy" of DefinitelyTyped/DefinitelyTyped#49953
since we copied the type definitions from DefinitlyTyped to fix the (types) regression introduced in #278.

The current types reference TypeScript's DOM typings with /// <reference lib="dom" />. This causes any module that imports xmldom to get the entire DOM typings, including globals like document.

With those globals defined we lose errors when accidentally referencing them, which can obviously cause crashes.

It'd be great if we would only provide the types that we use and export, like DOMImplementation, Document, Node, etc.

Related "main thread": #191

@karfau karfau added bug Something isn't working types Anything regarding Typescript labels Aug 20, 2021
@karfau karfau changed the title xmldom types include globals that don't exist in Node The current types include globals that don't exist in Node Aug 20, 2021
@karfau karfau added this to the after next release milestone Aug 21, 2021
@amacneil
Copy link

Despite this reference in index.d.ts, when upgrading from xmldom:0.6.0 to @xmldom/xmldom:0.7.2 typescript complained about missing dom libraries and I had to explicitly add "lib": ["dom"] to tsconfig.json.

I'm not sure how to explain this, but it seems that this line might be ignored for types included in a package (versus loading from DefinitelyTyped)?

@karfau
Copy link
Member Author

karfau commented Aug 24, 2021

@amacneil thank you for providing those details.
But I'm surprised by that, since I would have expected that even with version 0.6.0 and the types package, it would have been required to add lib: ["dom"] if you typescript target is not a browser.
The reference comment is still in place in the current type definition, so that didn't change, but from my understanding this doesn't tell ts to make the types available globally, it just acts like an import.
And I would be surprised if the types package would contain anything that has an impact on the typescript build config of downstream projects. But I guess we could check the content of the package to find it out.

Sadly I'm currently not aware how to improve/resolve this issue without completely changing how we define those types.

@amacneil
Copy link

I agree, it is surprising behavior. I created a repro here: https://github.com/amacneil/xmldom-types-repro

You can see the build output here: https://github.com/amacneil/xmldom-types-repro/actions/runs/1163442338

I found that for both the old and new version, if the package imports xmldom (or @xmldom/xmldom), the reference is applied and "lib": ["dom"] is not needed.

However, if @types/xmldom simply exists in the package.json (but is not imported in any files), typescript still honors the reference. This is not the case with bundled types in @xmldom/xmldom. That explains why we had to add "lib": ["dom"] to on our package (the hooks package did not use xmldom, but the types existed elsewhere in our monorepo so would have applied everywhere, which is bad!).

I don't know why this behavior happens, but at least this proves what is happening.

@defunctzombie
Copy link

One thing worth calling out is that typescript default behavior is to load all the type definitions in the @types folder. It considers the @types folder a typeRoot (https://www.typescriptlang.org/tsconfig). This might contribute to different behavior for triple slash reference directives.

@karfau
Copy link
Member Author

karfau commented Feb 20, 2022

Since we have not heard anybody having issue with this for quite a while, I will close this issue now.

If this is still an issue for you, please leave a comment here and I will reopen it.

@karfau karfau closed this as completed Feb 20, 2022
@karfau karfau removed this from the before 1.0.0 milestone Feb 20, 2022
@karfau karfau added the invalid This doesn't seem right label Feb 20, 2022
@tjhorner
Copy link

tjhorner commented Feb 27, 2024

Having issues with this. When importing @xmldom/xmldom anywhere in a project, the triple-slash reference to the dom lib makes DOM types available everywhere in my project, even when they aren't supposed to be. Our project has multiple different platform targets (service worker, DOM, etc.) so it's important for us to have compile-time safeguards against using APIs that may not exist at runtime.

Here is the relevant TypeScript issue: microsoft/TypeScript#33111

A common solution other library maintainers have landed on is to just vendor the necessary DOM types into their projects, for example: sindresorhus/is#93

An ugly workaround is to import the JS file directly, bypassing the type declarations:

import { DOMParser } from "@xmldom/xmldom/lib/index"

But then you won't get the typings.

@karfau
Copy link
Member Author

karfau commented Feb 28, 2024

@tjhorner thx for reporting this.
Can you please help us to create a reproduction for this?
The reason for closing the issue was missing reports/reproduction.
As soon as we have a reproduction I would love to add it to the checks of this repository.

@karfau karfau reopened this Feb 28, 2024
@defunctzombie
Copy link

@karfau Was the above reproduction repo posted by @amacneil not sufficient to reproduce?

image

@danon
Copy link

danon commented Mar 16, 2024

I encountered this issue myself. Obviously the types in @xmldom shouldn't reference TypeScript DOM, since they're not compatible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help-wanted External contributions welcome invalid This doesn't seem right types Anything regarding Typescript
Projects
Status: Done
Development

No branches or pull requests

5 participants