-
Notifications
You must be signed in to change notification settings - Fork 1.4k
docs(vanilla typescript): Fix the return type of the exec function #10360
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
Conversation
When working through the vanilla typescript example I found that the shape of the data actually returned by the fetch was `result.data.
|
@@ -181,6 +181,7 @@ We can build a simple wrapper around `fetch` that takes a `TypedDocumentString` | |||
returns a typed response. | |||
|
|||
```typescript filename="src/graphql/execute.ts" | |||
import { ExecutionResult } from 'graphql' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think most readers would be on the later versions of TypeScript which allows import type
. I think this is better in general 🙂
import { ExecutionResult } from 'graphql' | |
import type { ExecutionResult } from 'graphql' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! I will adjust
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjusted @eddeee888
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
I've verified this suggestion locally and it works 🎉
Description
When working through the vanilla typescript example I found that the shape of the data actually returned by the fetch was
result.data.allPeople
rather thanresult.allPeople
that the types suggests. This PR corrects that.Related #10359
Type of change
Please delete options that are not relevant.
Screenshots/Sandbox (if appropriate/relevant):
N/A
How Has This Been Tested?
N/A - though confirmed this change works by others #10359 (comment)
Checklist:
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...