Skip to content
This repository has been archived by the owner on Apr 14, 2023. It is now read-only.

Remove TData generic, fixing GraphQL 15 compile #1262

Closed
wants to merge 2 commits into from
Closed

Remove TData generic, fixing GraphQL 15 compile #1262

wants to merge 2 commits into from

Conversation

zackdotcomputer
Copy link

@zackdotcomputer zackdotcomputer commented Apr 6, 2020

In earlier versions of GraphQL, the ExecutionResult took a generic parameter, which it then typecast its return type as. We were, however, just sending { [key: string]: any } for this parameter in all internal cases.

As of GraphQL 15, that generic parameter has been removed, (see this PR)[https://github.com/graphql/graphql-js/pull/2490/commits], and the return type has been fixed to always be { [key: string]: any }.

Since this is already compatible with our default, this PR removes the generic parameter to bring our typing in line with GraphQL's typing. This is, however, a breaking change for anyone who was depending on this generic parameter to do their typecast for them.

TODO:

  • Make sure all of new logic is covered by tests and passes linting
  • Update CHANGELOG.md with your change

In earlier versions of GraphQL, the ExecutionResult took a generic parameter, which it then typecast its return type as. We were, however, just sending `{ [key: string]: any }` for this parameter in all internal cases.

As of GraphQL 15, that generic parameter has been removed, (see this PR)[https://github.com/graphql/graphql-js/pull/2490/commits], and the return type has been fixed to always be `{ [key: string]: any }`.

Since this is already compatible with our default, this PR removes the generic parameter to bring our typing in line with GraphQL's typing. This is, however, a breaking change for anyone who was depending on this generic parameter to do their typecast for them.
@apollo-cla
Copy link

@zackdotcomputer: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@zackdotcomputer
Copy link
Author

Signed CLA, updated changelog. This fixes #1261 .

@codecov-io
Copy link

codecov-io commented Apr 6, 2020

Codecov Report

Merging #1262 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1262   +/-   ##
=======================================
  Coverage   95.60%   95.60%           
=======================================
  Files          22       22           
  Lines        1116     1116           
  Branches      162      174   +12     
=======================================
  Hits         1067     1067           
  Misses         44       44           
  Partials        5        5           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a07942...87a2249. Read the comment docs.

@hwillson
Copy link
Member

hwillson commented Apr 9, 2020

Thanks for working on this @zackdotcomputer. We've decided to merge #1263 to address this instead, since it's backwards compatible. Thanks again!

@hwillson hwillson closed this Apr 9, 2020
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.

None yet

4 participants