-
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
Add root level resolvers support to avoidOptionals
#10077
Conversation
🦋 Changeset detectedLatest commit: 0e5f88a 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 |
|
||
export const DEFAULT_AVOID_OPTIONALS: AvoidOptionalsConfig = { | ||
export const DEFAULT_AVOID_OPTIONALS: NormalizedAvoidOptionalsConfig = { |
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.
NormalizedAvoidOptionalsConfig
is similar to AvoidOptionalsConfig
but all the fields are non-optional, making it much easier to code with
🚀 Snapshot Release (
|
Package | Version | Info |
---|---|---|
@graphql-codegen/visitor-plugin-common |
5.4.0-alpha-20240801152800-0e5f88a94d303ef7dd966bd20c3905047e9dd61f |
npm ↗︎ unpkg ↗︎ |
@graphql-codegen/typescript-document-nodes |
4.0.10-alpha-20240801152800-0e5f88a94d303ef7dd966bd20c3905047e9dd61f |
npm ↗︎ unpkg ↗︎ |
@graphql-codegen/gql-tag-operations |
4.0.10-alpha-20240801152800-0e5f88a94d303ef7dd966bd20c3905047e9dd61f |
npm ↗︎ unpkg ↗︎ |
@graphql-codegen/typescript-operations |
4.3.0-alpha-20240801152800-0e5f88a94d303ef7dd966bd20c3905047e9dd61f |
npm ↗︎ unpkg ↗︎ |
@graphql-codegen/typescript-resolvers |
4.3.0-alpha-20240801152800-0e5f88a94d303ef7dd966bd20c3905047e9dd61f |
npm ↗︎ unpkg ↗︎ |
@graphql-codegen/typed-document-node |
5.0.10-alpha-20240801152800-0e5f88a94d303ef7dd966bd20c3905047e9dd61f |
npm ↗︎ unpkg ↗︎ |
@graphql-codegen/typescript |
4.1.0-alpha-20240801152800-0e5f88a94d303ef7dd966bd20c3905047e9dd61f |
npm ↗︎ unpkg ↗︎ |
@graphql-codegen/client-preset |
4.4.0-alpha-20240801152800-0e5f88a94d303ef7dd966bd20c3905047e9dd61f |
npm ↗︎ unpkg ↗︎ |
@graphql-codegen/graphql-modules-preset |
4.0.10-alpha-20240801152800-0e5f88a94d303ef7dd966bd20c3905047e9dd61f |
npm ↗︎ unpkg ↗︎ |
@@ -1307,7 +1314,7 @@ export class BaseResolversVisitor< | |||
} | |||
|
|||
protected formatRootResolver(schemaTypeName: string, resolverType: string, declarationKind: DeclarationKind): string { | |||
return `${schemaTypeName}${this.config.avoidOptionals ? '' : '?'}: ${resolverType}${this.getPunctuation( | |||
return `${schemaTypeName}${this.config.avoidOptionals.resolvers ? '' : '?'}: ${resolverType}${this.getPunctuation( |
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.
The old implementation can cause a scenario where an empty object is passed in, but it'd be treated as truthy so it'd treat it as non-optional.
In this new implementation, we use resolvers
because it's the option that is used for all resolvers-related fields in the past.
@@ -682,7 +689,7 @@ export class BaseResolversVisitor< | |||
allResolversTypeName: getConfigValue(rawConfig.allResolversTypeName, 'Resolvers'), | |||
rootValueType: parseMapper(rawConfig.rootValueType || '{}', 'RootValueType'), | |||
namespacedImportName: getConfigValue(rawConfig.namespacedImportName, ''), | |||
avoidOptionals: getConfigValue(rawConfig.avoidOptionals, false), | |||
avoidOptionals: normalizeAvoidOptionals(rawConfig.avoidOptionals), |
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 plugin seems to be the only one that's not using normalizeAvoidOptionals
. This makes it consistent.
💻 Website PreviewThe latest changes are available as preview in: https://32b7e480.graphql-code-generator.pages.dev |
@@ -78,6 +80,8 @@ export interface ParsedResolversConfig extends ParsedConfig { | |||
resolversNonOptionalTypename: ResolversNonOptionalTypenameConfig; | |||
} | |||
|
|||
type FieldDefinitionPrintFn = (parentName: string, avoidResolverOptionals: boolean) => string | null; |
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 was REALLY confused why/how the field's name is a function. Found out it's because we do custom logic in FieldDefinition
function. I'm extracting the type here to be able to type it instead of using any
const avoidResolverOptionals = | ||
typeof this.config.avoidOptionals === 'object' | ||
? this.config.avoidOptionals?.resolvers | ||
: this.config.avoidOptionals === true; |
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 now pass this from caller. I found that this function is called in Interface fields and Object type fields
return false; | ||
})(); | ||
|
||
const fieldsContent = (node.fields as unknown as FieldDefinitionPrintFn[]).map(f => { |
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 have to inline node.fields as unknown as FieldDefinitionPrintFn[]
here because it's the easiest way to communicate where this function comes from. If there's a better way, let me know!
@@ -1720,21 +1736,23 @@ export class BaseResolversVisitor< | |||
const allTypesMap = this._schema.getTypeMap(); | |||
const implementingTypes: string[] = []; | |||
|
|||
this._collectedResolvers[node.name as any] = { | |||
const typeName = node.name as any as string; |
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.
Cleaning this up by limiting where we do as any as string
in this function
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.
There's quite a few tests related to avoidOptionals
so I'm creating this new file to keep the old ones and add new tests related to query, mutation and subscription.
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.
👍 nice
Description
Most (if not all of the time), we need to implement root-level resolvers, otherwise there'd be runtime errors.
This PR extends
avoidOptionals
to generate non-optional root-level resolvers.This will also be used in Server Preset to avoid having to use
NonNullable
as discussed hereType of change
Please delete options that are not relevant.
How Has This Been Tested?