Suggestion: Add typescript types to package

[karfau]

There are some issues with the typescript types provided by @types/xmldom, that are hard to resolve, since they rely on the DOM types provided by typescript:
DefinitelyTyped/DefinitelyTyped#49953

I think the easiest way to resolve that issue and maintain the types is to add doc comments to the code in lib and generate the types from it.

The main advantage is also that the types don't claim to support APIs that it actually doesn't support, which currently would lead to a runtime error.

[brodybits]

I think this sounds good. We will probably want to ask DefinitelyTyped to deprecate the @types/xmldom package. It would also be ideal to add tests for the TypeScript types, as I have seen in some packages by Sindre Sorhus.

[kachkaev]

Some thoughts on this following a recent change in library name: #271 (comment)

[karfau]

Thx to @kachkaev for pointing out that since we resolved #271 by changing our package name to @xmldom/xmldom the types provided by @types/xmldom no longer work.

Personally I would favour to generate the type definitions from our source, even if they are not yet ideal. Since we currently do not have any build pipeline, I think we should commit the generated types to version control.

I think a single generated file should be good enough since it should only make the public interface available.

I have been experimenting with testing type definitions in the past, but I'm not very experienced.

Any support is welcome.

[kachkaev]

@karfau I agree with you that providing the typings from source is the best solution. It minimizes the probability of discrepancies between the declarations and the implementation. It might take some time to setup though depending on the current state of the codebase, leaving folks with no typings for some time.

If you generally support the migration of typings into the library, how about doing this in two steps?

1. Manually add existing typings from DefinitelyTyped → index.d.ts
   --Release a patch version--
2. Change how the lib is built, thus providing 'true' typings from source
   --Release a patch or a minor version--

Happy to help with 1 at least.

[brodybits]

An alternative idea could be to first add the types and then add a typecheck step to verify the consistency. This could help us avoid the complexities and limitations of adding a build pipeline, making it easier to install directly from GitHub, for example.

By "typecheck step" it should be possible to use the TypeScript compiler to check consistency between types in the DTS and types in our JavaScript code with help from JSDoc comments, like I started contributing to Prettier.

[karfau]

@kachkaev

I disagree that we need to change how the lib s build to generate types (step 2): They can also be generated from JS files.

The types that are currently in DefinitlyTyped have a really bad issue DefinitelyTyped/DefinitelyTyped#49953

I would actually prefer to at least try to generate the types from our existing sources,
before starting to manually type things.

But of course such a step can also break build pipelines, so I agree that step 1 makes sense to fix the regression for now and basically "own the typing issues" by moving them into this repo.

PR for step one is very welcome.

[kachkaev]

👀 #283

[forty]

I see that @kachkaev beat me to it, but I was presisely going to do that :) thanks!

@karfau The sooner that 0.7.1 version can be published, the better (master as is looks perfect) so downstream TS projects can do the security update without losing the types (for example https://github.com/node-saml/node-saml/). Thanks again for everything!

[kachkaev]

@forty in the meantime, you can add declarations/@xmldom/xmldom/index.d.ts with the contents from the PR. This will provide @xmldom/xmldom with the locally defined typings. When 0.7.1 is released, the file can be removed.

[karfau]

@forty @kachkaev 0.7.1 has been released

[amacneil]

Seems like this issue can be closed since the original stated request ("Add typescript types to package") has been completed and released in v0.7.1?

[karfau]

@amacneil You are right, there is some redundancy between this one and #285, and I need to figure out how to best resolve it, since I would love to rather continue the conversation of how to resolve the issue here ... maybe

[amacneil]

I think you can close this issue and track follow up work in separate issues

[brodybits]

It looks to me like this: the original task in the title "Add typescript types to package" is done ... but from discussions it looks like we have a couple of loose ends:

• issue with globals - The current types include globals that don't exist in Node #285
• start using JSDoc comments for the types

I would personally favor closing this and continuing with new issues as needed. Or is there anything else that should be discussed here?