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

Upgrade TypeScript and typecheck tests #5165

Merged
merged 2 commits into from
May 5, 2021

Conversation

glasser
Copy link
Member

@glasser glasser commented May 4, 2021

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
    Configure VS Code to use the local TypeScript version 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.

@glasser glasser requested review from martijnwalraven and abernix May 4, 2021 00:30
@glasser glasser force-pushed the glasser/upgrade-ts-typecheck-tests branch 3 times, most recently from 5706c55 to caa6093 Compare May 4, 2021 21:33
@glasser glasser marked this pull request as ready for review May 4, 2021 21:33
@glasser
Copy link
Member Author

glasser commented May 4, 2021

@martijnwalraven OK, I made a bunch more changes, documented in the PR description, and I'm pretty happy with this. I squashed everything together including your change but I did preserve your commit message at the bottom of the single commit message / PR description. It looks huge but is somewhat repetitive so I hope you may be able to review it today?

@glasser glasser force-pushed the glasser/upgrade-ts-typecheck-tests branch from c789557 to 2d7e8e3 Compare May 4, 2021 22:38
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.
@glasser glasser force-pushed the glasser/upgrade-ts-typecheck-tests branch from 2d7e8e3 to 39263a3 Compare May 4, 2021 22:52
Copy link
Contributor

@martijnwalraven martijnwalraven left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all makes sense to me. I would have preferred separating some of these changes into individual commits for tractability (like the global TypeScript types workaround), but no strong feelings about that.

* require any particular build configuration to use from jest. See the
* README.md for an example of how to use this with jest.
*/
export async function runKeyValueCacheTests(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a fine solution for now, to avoid creating a separate testing package. An alternative would have been to define a separate TypeScript project within the same package, which would have allowed the test to depend on Jest without those types affecting the production code. One benefit of that is that it would keep the ability to report individual test failures, as opposed to the first assertion throwing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern is that I do want this to be easily available to other implementations outside of the repo, and the vague (perhaps flawed) research I tried to do on how to make a Jest suite "exportable" didn't turn up much. I didn't want to put a bunch of effort into something that might end up not working in different TS/Jest contexts when it seemed reasonably straightforward to write something that would definitely work all the time. Reporting individual test failures would be nice, but it's also like one page of code in a package that hasn't been a major focus of Apollo first-party development recently.

@@ -32,10 +33,10 @@ const typeDefs = gql`

const resolvers = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah will give that a quick shot

@@ -32,10 +33,10 @@ const typeDefs = gql`

const resolvers = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@glasser
Copy link
Member Author

glasser commented May 5, 2021

fwiw I thought about leaving your commit separate but having separate commits where intermediate commits don't pass CI wasn't as attractive...

@glasser glasser merged commit b9dc920 into release-3.0 May 5, 2021
@glasser glasser deleted the glasser/upgrade-ts-typecheck-tests branch May 5, 2021 17:01
glasser added a commit that referenced this pull request May 5, 2021
glasser added a commit that referenced this pull request May 5, 2021
Our tests use the deprecated `apollo-fetch` package. This depends on an old
version of the `cross-fetch` polyfill package which itself depends on a version
of `node-fetch` with a minor CVE. This makes `npm audit` noisy, which is sad.

There's no real vulnerability from `node-fetch` here since it's only used in
tests, but it would be nice to quiet `npm audit`. Plus, in #5165 we grudgingly
added `"dom"` to the `lib` in `tsconfig.test.base.json` just to support
`apollo-fetch`.

Updating `cross-fetch` in `apollo-fetch` is not enticing because it does pull in
major version bumps of dependent packages; I wouldn't want to release a new
version of a dead project that makes bad changes in some obscure contexts.

But `apollo-fetch` is a pretty simple package. So instead, I just inlined
`apollo-fetch` into `apollo-server-integration-testsuite`, deleted a bunch of
features we aren't using like batch support, and made it use the
`apollo-server-env` `fetch` rather than a global polyfill.

The only use of `apollo-fetch` that didn't already depend on
`apollo-server-integration-testsuite` was `apollo-server`, so add that
devDependency.
glasser added a commit that referenced this pull request May 5, 2021
Our tests use the deprecated `apollo-fetch` package. This depends on an old
version of the `cross-fetch` polyfill package which itself depends on a version
of `node-fetch` with a minor CVE. This makes `npm audit` noisy, which is sad.

There's no real vulnerability from `node-fetch` here since it's only used in
tests, but it would be nice to quiet `npm audit`. Plus, in #5165 we grudgingly
added `"dom"` to the `lib` in `tsconfig.test.base.json` just to support
`apollo-fetch`.

Updating `cross-fetch` in `apollo-fetch` is not enticing because it does pull in
major version bumps of dependent packages; I wouldn't want to release a new
version of a dead project that makes bad changes in some obscure contexts.

But `apollo-fetch` is a pretty simple package. So instead, I just inlined
`apollo-fetch` into `apollo-server-integration-testsuite`, deleted a bunch of
features we aren't using like batch support, and made it use the
`apollo-server-env` `fetch` rather than a global polyfill.

The only use of `apollo-fetch` that didn't already depend on
`apollo-server-integration-testsuite` was `apollo-server`, so add that
devDependency.

Fixes #5161.
glasser added a commit that referenced this pull request May 5, 2021
Our tests use the deprecated `apollo-fetch` package. This depends on an old
version of the `cross-fetch` polyfill package which itself depends on a version
of `node-fetch` with a minor CVE. This makes `npm audit` noisy, which is sad.

There's no real vulnerability from `node-fetch` here since it's only used in
tests, but it would be nice to quiet `npm audit`. Plus, in #5165 we grudgingly
added `"dom"` to the `lib` in `tsconfig.test.base.json` just to support
`apollo-fetch`.

Updating `cross-fetch` in `apollo-fetch` is not enticing because it does pull in
major version bumps of dependent packages; I wouldn't want to release a new
version of a dead project that makes bad changes in some obscure contexts.

But `apollo-fetch` is a pretty simple package. So instead, I just inlined
`apollo-fetch` into `apollo-server-integration-testsuite`, deleted a bunch of
features we aren't using like batch support, and made it use the
`apollo-server-env` `fetch` rather than a global polyfill.

The only use of `apollo-fetch` that didn't already depend on
`apollo-server-integration-testsuite` was `apollo-server`, so add that
devDependency.

Fixes #5161.
glasser added a commit to apollographql/federation that referenced this pull request Jul 12, 2021
- Incorporate the upgrade of apollo-server-env too (see #876)
- Remove `subscriptions: false` from tests and docs
- Remove `engine: false` from a test (though I'm not super sure what it
  was doing)
- Don't depend on the apollo-server-env global types (removed in
  apollographql/apollo-server#5165)
abernix added a commit that referenced this pull request Jul 12, 2021
…ion guide

Follows up on the release of Apollo Server 3 with changes from
#5165 which were
previously inadvertently omitted from both the Changelog and the migration
guide.
abernix added a commit that referenced this pull request Jul 12, 2021
…ion guide (#5460)

Follows up on the release of Apollo Server 3 with changes from
#5165 which were
previously inadvertently omitted from both the Changelog and the migration
guide.
glasser added a commit to apollographql/federation that referenced this pull request Jul 13, 2021
- Incorporate the upgrade of apollo-server-env too (see #876)
- Remove `subscriptions: false` from tests and docs
- Remove `engine: false` from a test (though I'm not super sure what it
  was doing)
- Don't depend on the apollo-server-env global types (removed in
  apollographql/apollo-server#5165)
- Remove a test of AS2-specific error handling behavior
- Delete some other stuff from tests until they pass

Co-authored-by: Renovate Bot <bot@renovateapp.com>
Co-authored-by: David Glasser <glasser@davidglasser.net>
Co-authored-by: Trevor Scheer <trevor.scheer@gmail.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants