-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Generate input types and output enums into target file #10527
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
Generate input types and output enums into target file #10527
Conversation
🦋 Changeset detectedLatest commit: 9cea149 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
| ); | ||
| this._declarationBlockConfig = { | ||
| ignoreExport: this.config.noExport, | ||
| enumNameValueSeparator: ' =', |
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 experimented with different enumType, this value appears to be required for other enumTypes to work.
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.
Yes you are right! I've got a similar fix in my follow-up Enum PR here: https://github.com/dotansimha/graphql-code-generator/pull/10525/files#diff-d0b5088bd638fefba8ba49f14b488769f115ef1daff777afbe718a4c5ecdf111R172
| const operationsResult = oldVisit(allAst, { leave: visitor }); | ||
|
|
||
| let operationsContent = operationsResult.definitions.join('\n'); | ||
| const operationsDefinitions = operationsResult.definitions; |
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 rearranged/simplified the code here - please check.
| operationsContent = operationsResult.definitions.concat(exportConsts).join('\n'); | ||
| } | ||
|
|
||
| if (config.globalNamespace) { |
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 moved this block to the end - to include all the generated code.
| `); | ||
| }); | ||
|
|
||
| it('try different ways to generate enums', async () => { |
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.
Oops sorry! I should have mentioned I'm working on the tests here: #10525
Since similar tests have been moved to standalone.enum.spec.ts, shall we remove the duplicates in this PR?
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.
Sure! Extra tests removed.
We can probably rearrange tests in a better way - to avoid this file getting cluttered.
|
Sorry! I caused some conflicts in this PR after merging #10525 🙏 |
| private getInputObjectDeclarationBlock(node: InputObjectTypeDefinitionNode): DeclarationBlock { | ||
| return new DeclarationBlock(this._declarationBlockConfig) | ||
| .export() | ||
| .asKind('type') | ||
| .withName(this.convertName(node)) | ||
| .withComment(node.description?.value) | ||
| .withBlock((node.fields || []).join('\n')); | ||
| } | ||
|
|
||
| private getInputObjectOneOfDeclarationBlock(node: InputObjectTypeDefinitionNode): DeclarationBlock { | ||
| const declarationKind = (node.fields?.length || 0) === 1 ? 'type' : 'type'; | ||
| return new DeclarationBlock(this._declarationBlockConfig) | ||
| .export() | ||
| .asKind(declarationKind) | ||
| .withName(this.convertName(node)) | ||
| .withComment(node.description?.value) | ||
| .withContent(`\n` + (node.fields || []).join('\n |')); | ||
| } | ||
|
|
||
| private isValidVisit(ancestors: any): boolean { | ||
| const currentVisitContext = this.getVisitorKindContextFromAncestors(ancestors); | ||
| const isVisitingInputType = currentVisitContext.includes(Kind.INPUT_OBJECT_TYPE_DEFINITION); | ||
| const isVisitingEnumType = currentVisitContext.includes(Kind.ENUM_TYPE_DEFINITION); | ||
|
|
||
| return isVisitingInputType || isVisitingEnumType; | ||
| } |
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.
With utility functions that can be shared between typescript-operations and base-types-visitor like these, I wonder if we should abstract them into visitor-plugin-common now or in a following PR? 🤔
Having them in visitor-plugin-common helps ensure they'd behave the same all the time. I've done the same for Enum utilities here : https://github.com/dotansimha/graphql-code-generator/pull/10525/files#r2585220345
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.
Sure, I'll move them to visitor-plugin-common!
Maybe after I fix all the bugs that I discover while running over the yelp codebase?
I am working on a quite tricky bug now with enum not being rendered from a fragment in a separate document.
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.
Sure! No rush.
I also put a FIXME or TODO on these utility functions in my PRs to refactor later
| const typeInfo = new TypeInfo(schema); | ||
| visit( | ||
| documentNode, | ||
| visitWithTypeInfo(typeInfo, { |
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.
Huh, this is my first time learning about this. Could you help me with the TLDR; of TypeInfo and how it helps us here?
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 am no expert on this as well. I relied on claude AI for this piece of code.
Having said that, here is what I think this code is doing:
- It looks like an operation’s AST doesn’t include field types - only names.
TypeInfois a stateful helper that tracks typing context while you walk the AST (https://www.graphql-js.org/api-v16/utilities/#typeinfo).visitWithTypeInfowires that context into a visitor. I have not found a working docs link onvisitWithTypeInfo, I guess our best bet is the docs in the source code which is located at the same place asTypeInfo: https://github.com/graphql/graphql-js/blob/next/src/utilities/TypeInfo.ts.
0b36e76 to
eca9224
Compare
eca9224 to
83ee745
Compare
83ee745 to
e89b353
Compare
ac4d132 to
e51f65e
Compare
e51f65e to
58a9ac9
Compare
| .withBlock((node.fields || []).join('\n')).string; | ||
| } | ||
|
|
||
| InputValueDefinition( |
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.
In typescript plugin, the logic to generate a Input Value string is spread across multiple methods:
InputObjectTypeDefinitionNamedTypeListTypeNonNull- And a couple of private methods
That was very confusing to both maintainers and contributors, and results in as any as string in many places.
This version brings all logic into one place method.
I'll see if this could be re-used between both plugins
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.
Excellent!
I like that you replace all these functions with just one!
However, I am bit concerned about IIFE that you do here. Because this code runs in a hot loop, there is a chance for this code to cause performance issue. The chance is low (it should be optimized by JIT), but still...
Is there a way to write this code without IIFE? In the worst case - we can split it into 3 functions (it is still better than the original version).
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 call! I've replaced the IIFEs with pure functions in 9cea149
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.
Great! Yes, this look safer.
Thanks!
* input types, input/output enums are generated to the target files * cleanup * better code * more tests * cleanup * better code * better tests * bugfixing for inner types and outer enums * bugfixing after merge * cleanup * cleanup * fix snapshots * fix type errors in presets/client * updated tests/examples * Add standalone.input.spec.ts and update standalone tests to TDD * Update operations/visitor.ts to satisfy tests * Revert Client Preset changes * Add oneOf directive for GraphQL 15 * Update changeset * Refactor IIFE --------- Co-authored-by: Igor Kusakov <[email protected]> Co-authored-by: Eddy Nguyen <[email protected]>
Description
Generates input types into target files.
Generates output enums into target files.
Related # #10496
Type of change
Please delete options that are not relevant.
How Has This Been Tested?