Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: Remove explicit return types from sdk methods #7592

Closed

Conversation

P4sca1
Copy link
Contributor

@P4sca1 P4sca1 commented Feb 28, 2022

Description

Previously, the return type of all SDK methods was declared manually. This can
cause the return type to become outdated when graphql-request gets updated.
This already happened, as graphql-request removed the errors attribute from the
return type (see graffle-js/graffle#174).
Also, data was marked as possibly undefined which is wrong. data always exists
on a successful response (failed requests throw a ClientError).

A sideeffect of this change is that the extensionsType config became obsolete
and has been removed. Because of this, I marked this change as a breaking change.

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

I removed tests for the now removed extensionsType config and ran yarn test -u. All tests are still passing. I also tested the change in a private project and did not experience any issues with the change.

@changeset-bot
Copy link

changeset-bot bot commented Feb 28, 2022

🦋 Changeset detected

Latest commit: df0e6e4

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Feb 28, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/theguild/graphql-code-generator/7WYNTu7jFLy24t9NwnFiqy7zVnUB
✅ Preview: https://graphql-code-generator-git-fork-p4sca1-fix-grap-b5892c-theguild.vercel.app

@charlypoly
Copy link
Contributor

Hi @P4sca1,

The extensionTypes config has been added quite recently, it seems a bit premature to simply drop it because of concern around the future typing changes of graphql-request.
Typing extensions when using rawRequest is a legit use-case.
In order to completely remove the extensionTypes option without dropping this use-case, a good approach would be to patch graphql-request to provide extensions generic type parameter.

For now, we should especially fix the data as a non-undefined property and make the typings evolve in time.

Copy link
Contributor

@charlypoly charlypoly left a comment

Choose a reason for hiding this comment

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

@charlypoly charlypoly added the waiting-for-answer Waiting for answer from author label Mar 17, 2022
@P4sca1
Copy link
Contributor Author

P4sca1 commented Mar 22, 2022

I agree, we should wait for graphql-request to provide the ability to change the extensions type, before making this change.
In the mean time, I will make a seperate PR to fix the current typings.

@P4sca1 P4sca1 mentioned this pull request Mar 22, 2022
4 tasks
@charlypoly
Copy link
Contributor

In the mean time, I will make a seperate PR to fix the current typings.

Thank you @P4sca1!

@charlypoly
Copy link
Contributor

@P4sca1, is this PR still relevant?

@P4sca1
Copy link
Contributor Author

P4sca1 commented Mar 22, 2022

No, this one is no longer needed.

@P4sca1 P4sca1 closed this Mar 22, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
waiting-for-answer Waiting for answer from author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants