feat: support TypedDocumentString as query argument#609
feat: support TypedDocumentString as query argument#609fpapado wants to merge 5 commits intooctokit:mainfrom
Conversation
|
👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with |
| If your query is represented as a `String & DocumentTypeDecoration`, for example as [`TypedDocumentString` from graphql-codegen and its client preset](https://the-guild.dev/graphql/codegen/docs/guides/vanilla-typescript), then you can get type-safety for the query's parameters and return values. | ||
|
|
||
| Assuming you have configured graphql-codegen, with [GitHub's GraphQL schema](https://docs.github.com/en/graphql/overview/public-schema), and an output under `./graphql`: |
There was a problem hiding this comment.
I'm trying to balance giving a complete guide to graphql-codegen and the GitHub GraphQL API, but maybe we can be more descriptive here 💭
| const innerQuery = | ||
| typeof query === "string" | ||
| ? query | ||
| : // Allows casting String & DocumentTypeDecoration<unknown, unknown> to | ||
| // string. This could be replaced with an instanceof check if we had | ||
| // access to a shared TypedDocumentString. Alternatively, we could use | ||
| // string & TypedDocumentDecoration<unknown, unknown> as the external | ||
| // interface, and push `.toString()` onto the caller, which might not | ||
| // be the worst idea. | ||
| String.prototype.isPrototypeOf(query) | ||
| ? query.toString() | ||
| : (query as RequestParameters); |
| // TODO: This setting is not compatible with the default definition of | ||
| // DocumentTypeDecoration from '@graphql-codegen-types/core'. That | ||
| // definition includes `{__apiType?: ...}` as an optional, so assigning it | ||
| // to an explicit `undefined` is impossible under | ||
| // exactOptionalPropertyTypes. We only need to work around this in tests, | ||
| // since that is where we use concrete examples of DocumentTypeDecoration. | ||
| "exactOptionalPropertyTypes": false |
There was a problem hiding this comment.
A bit cursed, but I do not know of a way to work around this from the call-site. Here is relevant part in graphql-typed-document-node
fpapado
force-pushed
the
feature/support-typed-document-string
branch
from
234c0a2 to
77edf5e
Compare
There was a problem hiding this comment.
Manually typing a trivial query is one thing, but what about big actual production queries and making changes to them? And the fact that graphql schema changes, stuff becomes nullable or non-nullable, enums get added, etc.
But lucky for you, I have a solution that works GREAT, is 100% automated and so it scales to arbitrarily large queries of arbitrary complexity.
See #596 (comment)
| { | ||
| repository: { issues: { edges: Array<{ node: { title: string } }> } }; | ||
| }, |
There was a problem hiding this comment.
I don't like that you have to manually type the query. You might as well do const response = graphql(...) as { repository: { issues: { edges: Array<{ node: { title: string } }> } } }, this is giving you nothing but false type safety.
There was a problem hiding this comment.
To clarify: this is a manual annotation that is added as part of the unit tests, in order to test the interface around TypeDocumentString, and to avoid running graphql-codegen as part of the test setup 👀 The expectation is fully that users will get TypeDocumentString types via graphql-codegen, and both the result and variables will be inferred (carried forward) when passed to octokit graphql's request function.
2 participants
Resolves #596
Before the change?
TypedDocumentStringwithgraphql-codegen, but would have to write their own wrapper aroundgraphql, in order to ensure type-safety. This is not a big deal to be honest, but having support out of the box simplifies ergonomics.After the change?
String & DocumentTypeDecoration<ResponseData, QueryVariables>. This the definition ofTypedDocumentStringfromgraphql-codegen/ graphql-typed-document-node.DocumentTypeDecorationitself is the lowest common denominator for type-safety in that ecosystem. If the user provides such a query, then we can have type-safety for the query parameters and return value.Pull request checklist
Does this introduce a breaking change?
Please see our docs on breaking changes to help!
This is not a breaking change. The API did not accept plain
TypedDocumentStrings previously, whereas now it does. I also took care to with the interface overrides, to preserveRequestParametersas optional, unless the query is aTypedDocumentStringwith required variables.Aside: I would be happy to contribute a few type-only tests for the public API interface. The overrides can lead to subtle changes, and I've found type-only tests to be useful to catch regressions in similar projects.
Open remarks
I am not fully happy with the shape here, and I have considered a couple of alternatives, that I could use your input on 👀
Lack of a shared
TypedDocumentStringtypeThe first issue is that
graphql-codegendoes not expose a sharedTypedDocumentStringtype from its core types, but rather inlines it in the codegen output. The type has a runtime representation (as a class), so it is a bit more complicated than the type-onlyTypedDocumentNode. This is discussed both in the original issue and a follow-up issue in graphql-typed-document-node.The type
String & DocumentTypeDecorationworks well-enough, becauseDocumentTypeDecorationhas the actionable types, but that means that the contract between this library and graphql-codegen output is less explicit. Alternative tools do not have access toTypedDocumentStringthe same way that they have access toTypedDocumentNode.String & DocumentTypeDecorationvsstring & DocumentTypeDecorationPartly due to the lack of a shared type, and partly because of the runtime class representation, we are currently doing a not-as-nice
String.prototype.isPrototypeOfcheck to duck-type theTypedDocumentString, and call.toString()on it. This could be nicer with atypeofcheck, but that is only part of my hesitation.An alternative would be to use
string & DocumentTypeDecorationas the interface.In practice, this means that instead of this:
...the caller would have to do:
This simplifies the internal implementation and reliance on the runtime representation of
TypedDocumentString, at the cost of some developer ergonomics.I might be overthinking this; I would be happy to implement it/document it whichever way you prefer 😇
Missing one implementation overload
This one is small: I did not implement a
TypedDocumentStringoverload for the query-as-request-parameter API shape. I am happy to implement it, but first I would like feedback on the above points, before going down that rabbit hole 😅These are all the open discussion points that I can think of. Please let me know if there is any other information that I can provide, and I'll get back to you. And thank you for all your work on octokit; it's been a joy to use over the years!