Definitelytyped: Introducing DOM typings to superagent can mask issues in node-only project

[fiznool]

If you know how to fix the issue, make a pull request instead.

• [x] I tried using the @types/superagent package and had problems.
• [x] I tried using the latest stable version of tsc. https://www.npmjs.com/package/typescript
• [x] I have a question that is inappropriate for StackOverflow. (Please ask any appropriate questions there).
• [x] [Mention](https://github.com/blog/821-mention-somebody-they-re-notified) the authors (see Definitions by: in index.d.ts) so they can respond.
  
  
  
  • Authors: @carnesen
  
  

---

With the merging of #36282, there is a problem. When using superagent in a node-only project, the introduction of the triple-slash directive

/// <reference lib="dom" />

results in the DOM typings being transparently added to the project. Since this is a node-only project, however, there is no DOM, and so code such as

window.setTimeout()

should result in a TypeScript error. Since the DOM typings are silently included, this is not the case, and can lead to subtle bugs in the codebase.

Is there a way that we can include node-only or browser-only typings into a project, so we can choose which to use?

[carnesen]

@fiznool, thank you for filing this ticket. I'll cc the rest of the superagent types contributors @vdhpieter, @NicoZelaya, @mxl, @paplorinc, @shreyjain1994, @beeequeue, @lukaselmer, @theQuazz, @ghostganz to see if they can help with this too.

I created a little test project and you're right that installing @types/superagent silently includes all the DOM types to the global ambient declarations, which is not safe for Node.js users. Similarly by virtue of the line /// <reference types="node" /> and the npm dependency on @types/node, it also silently adds all the Node.js types to the global ambient declarations which isn't safe for browser users. What do y'all think about creating two separate packages:

1) @types/superagent-node would depend explicitly on @types/node and look like:

/// <reference types="node" />

declare module "superagent" {
// Node.js-appropriate types
}

2) @types/superagent-dom would look like

/// <reference lib="dom" />

declare module "superagent" {
// Browser-appropriate types
}

I guess these would probably each depend on a third package @types/superagent which would have the truly isomorphic shared type declarations. (Declaration merging? I'm not 100% familiar with how that works https://www.typescriptlang.org/docs/handbook/declaration-merging.html)

What do you all think about that approach?

Do you know of any other prominent isomorphic/universal JavaScript packages that we could look at for guidance?

[carnesen]

@fiznool et al. Have you had a chance to consider the approach described above of introducing two separate @types packages, one for Node.js and one for browser?

[fiznool]

It’s a good idea but I haven’t explored it.

I have decided not to use supertest in my current project so this issue isn’t currently affecting me, hence I’m probably not the correct person to try and implement a fix.

[BeeeQueue]

To me it sounds more like there's a type in there that shouldn't be, and that this is just working around that issue rather than fixing it.

If XMLHttpRequest doesn't exist in Node it has to be using something else, so the type should be wrong.

IMO it would be better to have a duplicated/mocked type copied into the project rather than having two separate packages.

[Georift]

I've run into this today, it's quite non-obvious to find that superagent is the root cause. Here are some issues related to this problem from the Typescript project (microsoft/TypeScript#33111, microsoft/TypeScript#37775).

@carnesen I like the idea of the split types, it's unfortunate that the Typescript project doesn't have a path forward for us on this. I wonder how we can make it clear to people consuming this type that there exists two others @types/superagent-node & @types/superagent-dom?

Do we know yet what types are in conflict and which would need to belong to each package? In the first issue I linked (microsoft/TypeScript#33111) they talk of removing the reference to node & dom and extracting just the types we need. While it's a gross solution it would give us a path forward which we implement today to prevent global pollution.

I'm going to spend a little time on this second point and see where I can get. I suspect we're going to need the split packages as you've described if we run into a type conflict between node and dom.