Introducing DOM typings to superagent can mask issues in node-only project #41425

fiznool · 2020-01-06T14:08:58Z

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

I tried using the @types/superagent package and had problems.
I tried using the latest stable version of tsc. https://www.npmjs.com/package/typescript
I have a question that is inappropriate for StackOverflow. (Please ask any appropriate questions there).
Mention 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?

The text was updated successfully, but these errors were encountered:

carnesen · 2020-01-12T06:09:31Z

@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:

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

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

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

@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 · 2020-01-19T19:30:58Z

@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 · 2020-01-20T18:18:59Z

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 · 2020-01-20T18:34:12Z

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 · 2020-11-06T04:26:09Z

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.

daniel-white · 2020-11-18T23:20:09Z

Is there a reason that Response.xhr is even a thing on the type definitions? I would say that's a private member and should be omitted from the type definitions. See: https://github.com/visionmedia/superagent/blob/master/src/response-base.js
I don't see it for Request where superagent kinda has it there.

Georift · 2020-11-19T06:08:10Z

@daniel-white I agree it's probably only used vary rarely, I would be leaning toward making it xhr: any and remove reference to the XMLHttpRequest entirely.

@carnesen @beeequeue as the two active contributors on this issue could we get some thoughts on this?

@beeequeue I looked to duplicate the type in but it ended up being a massive number of lines (more than we have for superagent alone).

I believe option to leave something poorly typed (Reponse.xhr: any) is more prudent than introducing entirely incorrect types into an environment where they aren't present. (eg. Account is the global scope in node 😟)

beeequeue · 2020-11-20T15:31:56Z

I agree, I don't think a lot of users are using Response.xhr, and if they really desperately need it they can always re-introduce the type with a type enhancement.

declare module "superagent" {
  interface Response {
    xhr: XMLHttpRequest
  }
}

If we do this it would also be nice to update the types version to 6.0 to match superagent's current version. Then it would technically be a major version bump with a breaking change in it! 😄

I looked through the changelogs but couldn't see any types that had to be changed but if you decide to try it make sure to double check.

I don't have time to make a PR for it but I can review any that are opened.

carnesen · 2020-11-21T00:05:18Z

Since superagent is meant to be an isomorphic package, I agree that it shouldn't reference types from either @types/node nor from the TypeScript built-in DOM type library. I would support the approach of removing all references to those types even if it meant just replacing them with something non-specific like any. In the future, we could refine those anys to something more specific.

Georift · 2020-11-22T07:44:13Z

Great, thanks for the input. I'll have a look at making this change + the types needed to bump to v6 early this week.

We use exports from`apollo-server-env` to access the Fetch API to avoid depending on running in specific environments. While environments like Cloudflare Workers and Deno expose a Fetch API globally, in Node environments you'd generally like to avoid polyfilling the global context. There are also differences between the APIs available in these environments that we may want to account for with our own types. Unfortunately,`apollo-fetch` uses `cross-fetch` as a global polyfill (see https://github.com/apollographql/apollo-fetch/blob/0a0b8df241960dfa72abf3bd179e9d936265a7da/packages/apollo-fetch/src/apollo-fetch.ts#L17), and expects having Fetch API types defined globally. Because we use `apollo-fetch` in our tests, we've long used a `global.d.ts` in `apollo-server-env` to make the exported types available globally without depending on the `dom` lib types (because most of the types in there don't really make sense in non-browser environments and can lead to accidental usage). As it turns out however, we also use `superagent` in our tests, and `@types/superagent` now specifies an explicit dependency on `dom` lib. As TypeScript makes any global type declarations available throughout a project, that means our tests now get the `dom` lib types merged into their environment, resulting in conflicts with the types in `apollo-server-env`'s `global.d.ts`. Forcing anyone depending on packages like `@types/superagent` to have their environment extended with the `dom` lib types isn't great, and there are existing discussions about the implications of this for that package specifically (see DefinitelyTyped/DefinitelyTyped#41425). There is also a promising proposal for a `strictEnvironment` option in TypeScript that would help avoid these situations (see https://gist.github.com/RyanCavanaugh/702ebd1ca2fc060e58e634b4e30c1c1c). For now, I've solved this by reluctantly adding `dom` to `tsconfig.test.base.json`. We should be able to remove this when our dependencies get fixed or if `strictEnvironment` gets implemented. I also fixed some typing issues that ironically illustrate the danger of having these `dom` types available. Because the mocked version of `apollo-server-env` exported classes through a `const` declaration, that didn't actually export them as types, just as values (see microsoft/TypeScript#36348). But because we have similarly named types defined in `dom`, those were used instead. Using named exports in the `apollo-server-env` mock avoids this.

@martijnwalraven

Upgrading to TS 4 is generally useful, but it also lets use combine `composite` with `noEmit`, which lets us actually use `tsconfig.test.json`. This PR copies a few things we've done recently in the Federation repo. - Upgrade TypeScript, Jest, and Codecov. - Drop jest-cucumber dep (hasn't been used for a while). - Update VSCode task to use the "typescript" type. - Set enablePromptUseWorkspaceTsdk like in apollographql/federation#728 - Stop using moduleNameMapper in jest config, which seemed to create weird issues. Instead, just make sure that `npm run compile` happens before you run tests (at least when run via `npm test`) via a `pretest` script. - Fix a lot of compilation errors (mostly caused by actually typechecking tests, though some from the upgrade). Most of this is pretty localized, but some required more work (eg, replacing a `Map` that was being treated as a `KeyValueCache` with an actual `KeyValueCache` implementation). - I found the moduleNameMapper pulling in a top-level `__mocks__` to be confusing and also led to errors about trying to import files from outside a project. In fact in general I found `__mocks__` to be confusing. So: - I replaced our use of a custom `Date` mocker plus `jest.useFakeTimers` with `@sinonjs/fake-timers` (which does both and is what jest's is built on) - Replaced `__mocks__/apollo-server-env.ts` with a file that doesn't have a special `__mocks__` name - Did the same for the ioredis mock, and loaded it into `jest.mock` explicitly rather than implicitly - The thing where we imported a function that defined a jest test suite from the `__tests__` of `apollo-server-caching` was sort of weird already, and importing from a project that used `noEmit` didn't seem to work at all here. So I changed the testing function to just be a normal non-Jest-specific function exported from `apollo-server-caching` that can be used in Jest tests (or non-Jest tests for that matter) with a bit of preamble. (I changed its name too so it's obviously a different function.) Because closing the cache is now not part of the test, we didn't need `TestableKeyValueCache` any more so I dropped it. - isDirectiveDefined isn't exported and is now always called with an array, so remove code that runs if it doesn't get one and tests that that code works. - RESTDataSource used to document and test that you can override `baseURL` in a subclass with a getter. This is illegal in TS4 so we don't officially support it any more. Additionally, this PR removes global types from `apollo-server-env` (the `global.d.ts` file). Thanks to @martijnwalraven for figuring this out and writing the following: We use exports from `apollo-server-env` to access the Fetch API to avoid depending on running in specific environments. While environments like Cloudflare Workers and Deno expose a Fetch API globally, in Node environments you'd generally like to avoid polyfilling the global context. There are also differences between the APIs available in these environments that we may want to account for with our own types. Unfortunately,`apollo-fetch` uses `cross-fetch` as a global polyfill (see https://github.com/apollographql/apollo-fetch/blob/0a0b8df241960dfa72abf3bd179e9d936265a7da/packages/apollo-fetch/src/apollo-fetch.ts#L17), and expects having Fetch API types defined globally. Because we use `apollo-fetch` in our tests, we've long used a `global.d.ts` in `apollo-server-env` to make the exported types available globally without depending on the `dom` lib types (because most of the types in there don't really make sense in non-browser environments and can lead to accidental usage). As it turns out however, we also use `superagent` in our tests, and `@types/superagent` now specifies an explicit dependency on `dom` lib. As TypeScript makes any global type declarations available throughout a project, that means our tests now get the `dom` lib types merged into their environment, resulting in conflicts with the types in `apollo-server-env`'s `global.d.ts`. Forcing anyone depending on packages like `@types/superagent` to have their environment extended with the `dom` lib types isn't great, and there are existing discussions about the implications of this for that package specifically (see DefinitelyTyped/DefinitelyTyped#41425). There is also a promising proposal for a `strictEnvironment` option in TypeScript that would help avoid these situations (see https://gist.github.com/RyanCavanaugh/702ebd1ca2fc060e58e634b4e30c1c1c). For now, I've solved this by reluctantly adding `dom` to `tsconfig.test.base.json`. We should be able to remove this when our dependencies get fixed or if `strictEnvironment` gets implemented. I also fixed some typing issues that ironically illustrate the danger of having these `dom` types available. Because the mocked version of `apollo-server-env` exported classes through a `const` declaration, that didn't actually export them as types, just as values (see microsoft/TypeScript#36348). But because we have similarly named types defined in `dom`, those were used instead. Using named exports in the `apollo-server-env` mock avoids this.

@martijnwalraven

Upgrading to TS 4 is generally useful, but it also lets use combine `composite` with `noEmit`, which lets us actually use `tsconfig.test.json`. This PR copies a few things we've done recently in the Federation repo. - Upgrade TypeScript, Jest, and Codecov. - Drop jest-cucumber dep (hasn't been used for a while). - Update VSCode task to use the "typescript" type. - Set enablePromptUseWorkspaceTsdk like in apollographql/federation#728 - Stop using moduleNameMapper in jest config, which seemed to create weird issues. Instead, just make sure that `npm run compile` happens before you run tests (at least when run via `npm test`) via a `pretest` script. - Fix a lot of compilation errors (mostly caused by actually typechecking tests, though some from the upgrade). Most of this is pretty localized, but some required more work (eg, replacing a `Map` that was being treated as a `KeyValueCache` with an actual `KeyValueCache` implementation). - I found the moduleNameMapper pulling in a top-level `__mocks__` to be confusing and also led to errors about trying to import files from outside a project. In fact in general I found `__mocks__` to be confusing. So: - I replaced our use of a custom `Date` mocker plus `jest.useFakeTimers` with `@sinonjs/fake-timers` (which does both and is what jest's is built on) - Replaced `__mocks__/apollo-server-env.ts` with a file that doesn't have a special `__mocks__` name - Did the same for the ioredis mock, and loaded it into `jest.mock` explicitly rather than implicitly - The thing where we imported a function that defined a jest test suite from the `__tests__` of `apollo-server-caching` was sort of weird already, and importing from a project that used `noEmit` didn't seem to work at all here. So I changed the testing function to just be a normal non-Jest-specific function exported from `apollo-server-caching` that can be used in Jest tests (or non-Jest tests for that matter) with a bit of preamble. (I changed its name too so it's obviously a different function.) Because closing the cache is now not part of the test, we didn't need `TestableKeyValueCache` any more so I dropped it. - isDirectiveDefined isn't exported and is now always called with an array, so remove code that runs if it doesn't get one and tests that that code works. - RESTDataSource used to document and test that you can override `baseURL` in a subclass with a getter. This is illegal in TS4 so we don't officially support it any more. Additionally, this PR removes global types from `apollo-server-env` (the `global.d.ts` file). Thanks to @martijnwalraven for figuring this out and writing the following: We use exports from `apollo-server-env` to access the Fetch API to avoid depending on running in specific environments. While environments like Cloudflare Workers and Deno expose a Fetch API globally, in Node environments you'd generally like to avoid polyfilling the global context. There are also differences between the APIs available in these environments that we may want to account for with our own types. Unfortunately,`apollo-fetch` uses `cross-fetch` as a global polyfill (see https://github.com/apollographql/apollo-fetch/blob/0a0b8df241960dfa72abf3bd179e9d936265a7da/packages/apollo-fetch/src/apollo-fetch.ts#L17), and expects having Fetch API types defined globally. Because we use `apollo-fetch` in our tests, we've long used a `global.d.ts` in `apollo-server-env` to make the exported types available globally without depending on the `dom` lib types (because most of the types in there don't really make sense in non-browser environments and can lead to accidental usage). As it turns out however, we also use `superagent` in our tests, and `@types/superagent` now specifies an explicit dependency on `dom` lib. As TypeScript makes any global type declarations available throughout a project, that means our tests now get the `dom` lib types merged into their environment, resulting in conflicts with the types in `apollo-server-env`'s `global.d.ts`. Forcing anyone depending on packages like `@types/superagent` to have their environment extended with the `dom` lib types isn't great, and there are existing discussions about the implications of this for that package specifically (see DefinitelyTyped/DefinitelyTyped#41425). There is also a promising proposal for a `strictEnvironment` option in TypeScript that would help avoid these situations (see https://gist.github.com/RyanCavanaugh/702ebd1ca2fc060e58e634b4e30c1c1c). For now, I've solved this by reluctantly adding `dom` to `tsconfig.test.base.json`. We should be able to remove this when our dependencies get fixed or if `strictEnvironment` gets implemented. I also fixed some typing issues that ironically illustrate the danger of having these `dom` types available. Because the mocked version of `apollo-server-env` exported classes through a `const` declaration, that didn't actually export them as types, just as values (see microsoft/TypeScript#36348). But because we have similarly named types defined in `dom`, those were used instead. Using named exports in the `apollo-server-env` mock avoids this. Fixes #5119.

@martijnwalraven

Upgrading to TS 4 is generally useful, but it also lets use combine `composite` with `noEmit`, which lets us actually use `tsconfig.test.json`. This PR copies a few things we've done recently in the Federation repo. - Upgrade TypeScript, Jest, and Codecov. - Drop jest-cucumber dep (hasn't been used for a while). - Update VSCode task to use the "typescript" type. - Set enablePromptUseWorkspaceTsdk like in apollographql/federation#728 - Stop using moduleNameMapper in jest config, which seemed to create weird issues. Instead, just make sure that `npm run compile` happens before you run tests (at least when run via `npm test`) via a `pretest` script. - Fix a lot of compilation errors (mostly caused by actually typechecking tests, though some from the upgrade). Most of this is pretty localized, but some required more work (eg, replacing a `Map` that was being treated as a `KeyValueCache` with an actual `KeyValueCache` implementation). - I found the moduleNameMapper pulling in a top-level `__mocks__` to be confusing and also led to errors about trying to import files from outside a project. In fact in general I found `__mocks__` to be confusing. So: - I replaced our use of a custom `Date` mocker plus `jest.useFakeTimers` with `@sinonjs/fake-timers` (which does both and is what jest's is built on) - Replaced `__mocks__/apollo-server-env.ts` with a file that doesn't have a special `__mocks__` name - Did the same for the ioredis mock, and loaded it into `jest.mock` explicitly rather than implicitly - The thing where we imported a function that defined a jest test suite from the `__tests__` of `apollo-server-caching` was sort of weird already, and importing from a project that used `noEmit` didn't seem to work at all here. So I changed the testing function to just be a normal non-Jest-specific function exported from `apollo-server-caching` that can be used in Jest tests (or non-Jest tests for that matter) with a bit of preamble. (I changed its name too so it's obviously a different function.) Because closing the cache is now not part of the test, we didn't need `TestableKeyValueCache` any more so I dropped it. - isDirectiveDefined isn't exported and is now always called with an array, so remove code that runs if it doesn't get one and tests that that code works. - RESTDataSource used to document and test that you can override `baseURL` in a subclass with a getter. This is illegal in TS4 so we don't officially support it any more. Additionally, this PR removes global types from `apollo-server-env` (the `global.d.ts` file). Thanks to @martijnwalraven for figuring this out and writing the following: We use exports from `apollo-server-env` to access the Fetch API to avoid depending on running in specific environments. While environments like Cloudflare Workers and Deno expose a Fetch API globally, in Node environments you'd generally like to avoid polyfilling the global context. There are also differences between the APIs available in these environments that we may want to account for with our own types. Unfortunately,`apollo-fetch` uses `cross-fetch` as a global polyfill (see https://github.com/apollographql/apollo-fetch/blob/0a0b8df241960dfa72abf3bd179e9d936265a7da/packages/apollo-fetch/src/apollo-fetch.ts#L17), and expects having Fetch API types defined globally. Because we use `apollo-fetch` in our tests, we've long used a `global.d.ts` in `apollo-server-env` to make the exported types available globally without depending on the `dom` lib types (because most of the types in there don't really make sense in non-browser environments and can lead to accidental usage). As it turns out however, we also use `superagent` in our tests, and `@types/superagent` now specifies an explicit dependency on `dom` lib. As TypeScript makes any global type declarations available throughout a project, that means our tests now get the `dom` lib types merged into their environment, resulting in conflicts with the types in `apollo-server-env`'s `global.d.ts`. Forcing anyone depending on packages like `@types/superagent` to have their environment extended with the `dom` lib types isn't great, and there are existing discussions about the implications of this for that package specifically (see DefinitelyTyped/DefinitelyTyped#41425). There is also a promising proposal for a `strictEnvironment` option in TypeScript that would help avoid these situations (see https://gist.github.com/RyanCavanaugh/702ebd1ca2fc060e58e634b4e30c1c1c). For now, I've solved this by reluctantly adding `dom` to `tsconfig.test.base.json`. We should be able to remove this when our dependencies get fixed or if `strictEnvironment` gets implemented. I also fixed some typing issues that ironically illustrate the danger of having these `dom` types available. Because the mocked version of `apollo-server-env` exported classes through a `const` declaration, that didn't actually export them as types, just as values (see microsoft/TypeScript#36348). But because we have similarly named types defined in `dom`, those were used instead. Using named exports in the `apollo-server-env` mock avoids this. Fixes #5119.

@martijnwalraven

Upgrading to TS 4 is generally useful, but it also lets use combine `composite` with `noEmit`, which lets us actually use `tsconfig.test.json`. This PR copies a few things we've done recently in the Federation repo. - Upgrade TypeScript, Jest, and Codecov. - Drop jest-cucumber dep (hasn't been used for a while). - Update VSCode task to use the "typescript" type. - Set enablePromptUseWorkspaceTsdk like in apollographql/federation#728 - Stop using moduleNameMapper in jest config, which seemed to create weird issues. Instead, just make sure that `npm run compile` happens before you run tests (at least when run via `npm test`) via a `pretest` script. - Fix a lot of compilation errors (mostly caused by actually typechecking tests, though some from the upgrade). Most of this is pretty localized, but some required more work (eg, replacing a `Map` that was being treated as a `KeyValueCache` with an actual `KeyValueCache` implementation). - I found the moduleNameMapper pulling in a top-level `__mocks__` to be confusing and also led to errors about trying to import files from outside a project. In fact in general I found `__mocks__` to be confusing. So: - I replaced our use of a custom `Date` mocker plus `jest.useFakeTimers` with `@sinonjs/fake-timers` (which does both and is what jest's is built on) - Replaced `__mocks__/apollo-server-env.ts` with a file that doesn't have a special `__mocks__` name - Did the same for the ioredis mock, and loaded it into `jest.mock` explicitly rather than implicitly - The thing where we imported a function that defined a jest test suite from the `__tests__` of `apollo-server-caching` was sort of weird already, and importing from a project that used `noEmit` didn't seem to work at all here. So I changed the testing function to just be a normal non-Jest-specific function exported from `apollo-server-caching` that can be used in Jest tests (or non-Jest tests for that matter) with a bit of preamble. (I changed its name too so it's obviously a different function.) Because closing the cache is now not part of the test, we didn't need `TestableKeyValueCache` any more so I dropped it. - isDirectiveDefined isn't exported and is now always called with an array, so remove code that runs if it doesn't get one and tests that that code works. - RESTDataSource used to document and test that you can override `baseURL` in a subclass with a getter. This is illegal in TS4 so we don't officially support it any more. Additionally, this PR removes global types from `apollo-server-env` (the `global.d.ts` file). Thanks to @martijnwalraven for figuring this out and writing the following: We use exports from `apollo-server-env` to access the Fetch API to avoid depending on running in specific environments. While environments like Cloudflare Workers and Deno expose a Fetch API globally, in Node environments you'd generally like to avoid polyfilling the global context. There are also differences between the APIs available in these environments that we may want to account for with our own types. Unfortunately,`apollo-fetch` uses `cross-fetch` as a global polyfill (see https://github.com/apollographql/apollo-fetch/blob/0a0b8df241960dfa72abf3bd179e9d936265a7da/packages/apollo-fetch/src/apollo-fetch.ts#L17), and expects having Fetch API types defined globally. Because we use `apollo-fetch` in our tests, we've long used a `global.d.ts` in `apollo-server-env` to make the exported types available globally without depending on the `dom` lib types (because most of the types in there don't really make sense in non-browser environments and can lead to accidental usage). As it turns out however, we also use `superagent` in our tests, and `@types/superagent` now specifies an explicit dependency on `dom` lib. As TypeScript makes any global type declarations available throughout a project, that means our tests now get the `dom` lib types merged into their environment, resulting in conflicts with the types in `apollo-server-env`'s `global.d.ts`. Forcing anyone depending on packages like `@types/superagent` to have their environment extended with the `dom` lib types isn't great, and there are existing discussions about the implications of this for that package specifically (see DefinitelyTyped/DefinitelyTyped#41425). There is also a promising proposal for a `strictEnvironment` option in TypeScript that would help avoid these situations (see https://gist.github.com/RyanCavanaugh/702ebd1ca2fc060e58e634b4e30c1c1c). For now, I've solved this by reluctantly adding `dom` to `tsconfig.test.base.json`. We should be able to remove this when our dependencies get fixed or if `strictEnvironment` gets implemented. I also fixed some typing issues that ironically illustrate the danger of having these `dom` types available. Because the mocked version of `apollo-server-env` exported classes through a `const` declaration, that didn't actually export them as types, just as values (see microsoft/TypeScript#36348). But because we have similarly named types defined in `dom`, those were used instead. Using named exports in the `apollo-server-env` mock avoids this. Fixes #5119.

orta · 2021-06-07T13:09:24Z

Hi thread, we're moving DefinitelyTyped to use GitHub Discussions for conversations the @types modules in DefinitelyTyped.

To help with the transition, we're closing all issues which haven't had activity in the last 6 months, which includes this issue. If you think closing this issue is a mistake, please pop into the TypeScript Community Discord and mention the issue in the definitely-typed channel.

ab-pm · 2021-09-06T15:03:25Z

Was moved to #55382

@martijnwalraven

Upgrading to TS 4 is generally useful, but it also lets use combine `composite` with `noEmit`, which lets us actually use `tsconfig.test.json`. This PR copies a few things we've done recently in the Federation repo. - Upgrade TypeScript, Jest, and Codecov. - Drop jest-cucumber dep (hasn't been used for a while). - Update VSCode task to use the "typescript" type. - Set enablePromptUseWorkspaceTsdk like in apollographql/federation#728 - Stop using moduleNameMapper in jest config, which seemed to create weird issues. Instead, just make sure that `npm run compile` happens before you run tests (at least when run via `npm test`) via a `pretest` script. - Fix a lot of compilation errors (mostly caused by actually typechecking tests, though some from the upgrade). Most of this is pretty localized, but some required more work (eg, replacing a `Map` that was being treated as a `KeyValueCache` with an actual `KeyValueCache` implementation). - I found the moduleNameMapper pulling in a top-level `__mocks__` to be confusing and also led to errors about trying to import files from outside a project. In fact in general I found `__mocks__` to be confusing. So: - I replaced our use of a custom `Date` mocker plus `jest.useFakeTimers` with `@sinonjs/fake-timers` (which does both and is what jest's is built on) - Replaced `__mocks__/apollo-server-env.ts` with a file that doesn't have a special `__mocks__` name - Did the same for the ioredis mock, and loaded it into `jest.mock` explicitly rather than implicitly - The thing where we imported a function that defined a jest test suite from the `__tests__` of `apollo-server-caching` was sort of weird already, and importing from a project that used `noEmit` didn't seem to work at all here. So I changed the testing function to just be a normal non-Jest-specific function exported from `apollo-server-caching` that can be used in Jest tests (or non-Jest tests for that matter) with a bit of preamble. (I changed its name too so it's obviously a different function.) Because closing the cache is now not part of the test, we didn't need `TestableKeyValueCache` any more so I dropped it. - isDirectiveDefined isn't exported and is now always called with an array, so remove code that runs if it doesn't get one and tests that that code works. - RESTDataSource used to document and test that you can override `baseURL` in a subclass with a getter. This is illegal in TS4 so we don't officially support it any more. Additionally, this PR removes global types from `apollo-server-env` (the `global.d.ts` file). Thanks to @martijnwalraven for figuring this out and writing the following: We use exports from `apollo-server-env` to access the Fetch API to avoid depending on running in specific environments. While environments like Cloudflare Workers and Deno expose a Fetch API globally, in Node environments you'd generally like to avoid polyfilling the global context. There are also differences between the APIs available in these environments that we may want to account for with our own types. Unfortunately,`apollo-fetch` uses `cross-fetch` as a global polyfill (see https://github.com/apollographql/apollo-fetch/blob/0a0b8df241960dfa72abf3bd179e9d936265a7da/packages/apollo-fetch/src/apollo-fetch.ts#L17), and expects having Fetch API types defined globally. Because we use `apollo-fetch` in our tests, we've long used a `global.d.ts` in `apollo-server-env` to make the exported types available globally without depending on the `dom` lib types (because most of the types in there don't really make sense in non-browser environments and can lead to accidental usage). As it turns out however, we also use `superagent` in our tests, and `@types/superagent` now specifies an explicit dependency on `dom` lib. As TypeScript makes any global type declarations available throughout a project, that means our tests now get the `dom` lib types merged into their environment, resulting in conflicts with the types in `apollo-server-env`'s `global.d.ts`. Forcing anyone depending on packages like `@types/superagent` to have their environment extended with the `dom` lib types isn't great, and there are existing discussions about the implications of this for that package specifically (see DefinitelyTyped/DefinitelyTyped#41425). There is also a promising proposal for a `strictEnvironment` option in TypeScript that would help avoid these situations (see https://gist.github.com/RyanCavanaugh/702ebd1ca2fc060e58e634b4e30c1c1c). For now, I've solved this by reluctantly adding `dom` to `tsconfig.test.base.json`. We should be able to remove this when our dependencies get fixed or if `strictEnvironment` gets implemented. I also fixed some typing issues that ironically illustrate the danger of having these `dom` types available. Because the mocked version of `apollo-server-env` exported classes through a `const` declaration, that didn't actually export them as types, just as values (see microsoft/TypeScript#36348). But because we have similarly named types defined in `dom`, those were used instead. Using named exports in the `apollo-server-env` mock avoids this. Fixes #5119.

fiznool mentioned this issue Jan 6, 2020

@types/superagent error TS2304: Cannot find name 'XMLHttpRequest'. #12044

Closed

4 tasks

Georift mentioned this issue Nov 6, 2020

Node module dependencies that use a triple-slash directive to reference libs poison the global scope microsoft/TypeScript#33111

Open

glasser mentioned this issue May 4, 2021

Upgrade TypeScript and typecheck tests apollographql/apollo-server#5165

Merged

This was referenced May 10, 2021

@types/superagent requires @types/node which defines Node environment where browser environment is expected #19646

Closed

Omitting "dom" from lib should enforce excluding lib.dom.d.ts (for nodejs) microsoft/TypeScript#43990

Closed

orta closed this as completed Jun 7, 2021

yo1dog mentioned this issue Sep 13, 2021

Typings polutes global space with DOM types node-fetch/node-fetch#1285

Closed

alecgibson mentioned this issue Dec 9, 2021

[superagent] remove dom typings #57641

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introducing DOM typings to superagent can mask issues in node-only project #41425

Introducing DOM typings to superagent can mask issues in node-only project #41425

fiznool commented Jan 6, 2020

carnesen commented Jan 12, 2020

carnesen commented Jan 19, 2020

fiznool commented Jan 20, 2020

beeequeue commented Jan 20, 2020

Georift commented Nov 6, 2020

daniel-white commented Nov 18, 2020

Georift commented Nov 19, 2020

beeequeue commented Nov 20, 2020

carnesen commented Nov 21, 2020

Georift commented Nov 22, 2020

orta commented Jun 7, 2021

ab-pm commented Sep 6, 2021

Introducing DOM typings to superagent can mask issues in node-only project #41425

Introducing DOM typings to superagent can mask issues in node-only project #41425

Comments

fiznool commented Jan 6, 2020

carnesen commented Jan 12, 2020

carnesen commented Jan 19, 2020

fiznool commented Jan 20, 2020

beeequeue commented Jan 20, 2020

Georift commented Nov 6, 2020

daniel-white commented Nov 18, 2020

Georift commented Nov 19, 2020

beeequeue commented Nov 20, 2020

carnesen commented Nov 21, 2020

Georift commented Nov 22, 2020

orta commented Jun 7, 2021

ab-pm commented Sep 6, 2021