feat: add types to graphql in TS outputs #620
Conversation
| get typeDefs() { | ||
| if (this.language === 'typescript' && this.opTypeName) { | ||
| return [ | ||
| `import * as APITypes from '../API';`, |
There was a problem hiding this comment.
Is the location of API.ts stable relative to the generated graphql path? I'm guessing not. How should I calculate the correct relative path here?
There was a problem hiding this comment.
No it is not, customers can choose any custom path relative to the project root when they do "amplify add codegen".
| ${variablesTypeName}, | ||
| ${resultTypeName} | ||
| >;`; | ||
| } |
There was a problem hiding this comment.
I was initially thinking this was very gross. But, now I'm wondering if the name I receive here (from the statements map) is actually all the info I need, minus the casing.
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## main #620 +/- ##
==========================================
+ Coverage 85.93% 85.96% +0.02%
==========================================
Files 152 152
Lines 7489 7502 +13
Branches 1956 1959 +3
==========================================
+ Hits 6436 6449 +13
Misses 960 960
Partials 93 93
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| const variablesTypeName = `APITypes.${titleCasedName}${this.opTypeName}Variables`; | ||
| const resultTypeName = `APITypes.${titleCasedName}${this.opTypeName}`; |
There was a problem hiding this comment.
I'll have a rev by standup tomorrow that uses interfaceNameFromOperation + a new interfaceVariablesNameFromOperation instead of re-deriving those names here. If I'm reading things correctly, that should guard against future mismatches due to drift.
There was a problem hiding this comment.
Added as another PR to compare/contrast: https://github.com/aws-amplify/amplify-codegen/pull/623/files
| if (!this.opTypeName) return ''; | ||
| if (this.language !== 'typescript') return ''; | ||
|
|
||
| const titleCasedName = `${name[0].toUpperCase()}${name.slice(1)}`; |
There was a problem hiding this comment.
If I'm reading things right, what I'm deriving here is actually name from the ops arrays earlier in the chain. It looks safe to use this name and just fix casing, but in case I'm wrong and to prevent drift, I'm looking at adding an optional flag to generateGraphQLDocuments to return something like Map<string, {query: string, meta: { ...}}> when needed.
There was a problem hiding this comment.
Added as another PR to compare/contrast: https://github.com/aws-amplify/amplify-codegen/pull/623/files
|
Using this one instead: #623 |
Description of changes
Let's discuss the general approach before we worry about the nits.
In order to add types to the generated graphql for TypeScript, we need to know the underlying operation type (
Query | Mutation | Subscription) and specific operation name (CreateBlog,ListBlogs, etc.) at the time graphql operations are embeded/formatted for the language as variable assignments. This proposal infers the specific operation name from the key value in thestatementsmap.The alternative would be to weave original type names through the chain — this seems more robust. But, if the type name is effectively captured in the
statementsmap anyway, perhaps this is unnecessary.I'm looking for high level guidance from codegen folks before proceeding.
Issue #, if available
Description of how you validated changes
Checklist
yarn testpassesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.