-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[typescript-resolvers] Extract interface types into ResolversInterfaceTypes + add interface to resolversNonOptionalTypename + simplify ResolversUnionTypes #9229
Conversation
🦋 Changeset detectedLatest commit: 9a5572c The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 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 |
🚀 Snapshot Release (
|
Package | Version | Info |
---|---|---|
@graphql-codegen/visitor-plugin-common |
3.2.0-alpha-20230423100039-9a5572c84 |
npm ↗︎ unpkg ↗︎ |
@graphql-codegen/typescript-document-nodes |
3.0.5-alpha-20230423100039-9a5572c84 |
npm ↗︎ unpkg ↗︎ |
@graphql-codegen/gql-tag-operations |
3.0.2-alpha-20230423100039-9a5572c84 |
npm ↗︎ unpkg ↗︎ |
@graphql-codegen/typescript-operations |
3.0.5-alpha-20230423100039-9a5572c84 |
npm ↗︎ unpkg ↗︎ |
@graphql-codegen/typescript-resolvers |
3.3.0-alpha-20230423100039-9a5572c84 |
npm ↗︎ unpkg ↗︎ |
@graphql-codegen/typed-document-node |
4.0.2-alpha-20230423100039-9a5572c84 |
npm ↗︎ unpkg ↗︎ |
@graphql-codegen/typescript |
3.0.5-alpha-20230423100039-9a5572c84 |
npm ↗︎ unpkg ↗︎ |
@graphql-codegen/client-preset |
3.0.2-alpha-20230423100039-9a5572c84 |
npm ↗︎ unpkg ↗︎ |
@graphql-codegen/graphql-modules-preset |
3.1.4-alpha-20230423100039-9a5572c84 |
npm ↗︎ unpkg ↗︎ |
💻 Website PreviewThe latest changes are available as preview in: https://1aadfaa1.graphql-code-generator.pages.dev |
b418594
to
8f0a018
Compare
@@ -632,13 +634,15 @@ export class BaseResolversVisitor< | |||
applyWrapper: type => this.applyResolverTypeWrapper(type), | |||
clearWrapper: type => this.clearResolverTypeWrapper(type), | |||
getTypeToUse: name => this.getTypeToUse(name), | |||
currentType: 'ResolversTypes', |
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.
We are using currentType
as a generic so we don't have to create two types like ResolversUnionTypes
and ResolversUnionParentTypes
} else if (isInterfaceType(schemaType)) { | ||
this._hasReferencedResolversInterfaceTypes = true; | ||
const type = this.convertName('ResolversInterfaceTypes'); | ||
const generic = this.convertName(currentType); | ||
prev[typeName] = applyWrapper(`${type}<${generic}>['${typeName}']`); | ||
return prev; |
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.
This Interface block must be below hasDefaultMapper && !hasPlaceholder(this.config.defaultMapper.type)
if block.
Otherwise, the default mapper case with no placeholder e.g. config.defaultMapper: any
will incorrectly generate ResolversInterfaceTypes
packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts
Outdated
Show resolved
Hide resolved
return new DeclarationBlock(this._declarationBlockConfig) | ||
.export() | ||
.asKind(declarationKind) | ||
.withName(this.convertName('ResolversInterfaceTypes'), `<RefType extends Record<string, unknown>>`) |
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.
RefType
generic will be ResolversTypes
and ResolversParentTypes
in the final generated type. This will be used to refer back to the original types in cases
e.g. omitting the original field because it's a union
AnotherNode: ResolverTypeWrapper<any>; | ||
WithChild: ResolverTypeWrapper<any>; | ||
WithChildren: ResolverTypeWrapper<any>; |
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 the case of default mapper without placeholder, we can treat interface implementing types the same as how union treats it: typing it as ResolverTypeWrapper<DefaultMapper>
instead of ResolversTypes['typeA'] | ResolversTypes['typeB']
The reason being...
ResolversTypes['typeA'] | ResolversTypes['typeB']
resolves to...
ResolverTypeWrapper<DefaultMapper> | ResolverTypeWrapper<DefaultMapper>
so we can simplify it to just ResolverTypeWrapper<DefaultMapper>
553afcd
to
4d2d9b6
Compare
3cf04c7
to
5d3298f
Compare
5d3298f
to
9a5572c
Compare
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.
LGTM. Let's also wait for @saihaj to review
sorry for the delay @eddeee888 🥲 |
Description
1.
ResolversInterfaceTypes
is a new type that keeps track of a GraphQL interface and its implementing typesFor example, consider this schema:
The generated types will look like this:
The
RefType
generic is used to reference back toResolversTypes
andResolversParentTypes
in some cases such as field returning a Union.2.
resolversNonOptionalTypename
affectsResolversInterfaceTypes
Using the schema above, if we use
resolversNonOptionalTypename
option:Then, the generated type looks like this:
3. Simplify
ResolversUnionTypes
andResolversUnionParentTypes
with genericPreviously, we create
ResolversUnionTypes
andResolversUnionParentTypes
since each nested union member field may refer back toResolversTypes
andResolversParentTypes
respectively.This PR simplifies it by using a generic (similar to
ResolversInterfaceTypes
)Type of change
How Has This Been Tested?
TODO
Promise<T> | T
CharacterNode
in the example repo) isT
i.e. not a promiseTest Environment:
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...