Skip to content

[resolvers] CODEGEN-500: Support semanticNonNull custom directive #10315

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

Merged
merged 10 commits into from
Mar 21, 2025

Conversation

eddeee888
Copy link
Collaborator

@eddeee888 eddeee888 commented Mar 16, 2025

Related CODEGEN-500

Description

Based on #10159
Related to #10151

When @semanticNonNull custom directive is applied to a nullable field:

  • if the type not a list, we make the field non-nullable
  • if the type is a list, we make the list non-nullable, each item in the list is not affected

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • Unit test

Copy link

changeset-bot bot commented Mar 16, 2025

🦋 Changeset detected

Latest commit: 1cf1924

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages
Name Type
@graphql-codegen/visitor-plugin-common Minor
@graphql-codegen/typescript-resolvers Minor
@graphql-codegen/typescript-document-nodes Patch
@graphql-codegen/gql-tag-operations Patch
@graphql-codegen/typescript-operations Patch
@graphql-codegen/typed-document-node Patch
@graphql-codegen/typescript Patch
@graphql-codegen/graphql-modules-preset Patch
@graphql-codegen/client-preset Patch

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

@eddeee888 eddeee888 changed the title [resolvers] CODEGEN-500 Support semantic non null [resolvers] CODEGEN-500: Support semantic non null Mar 16, 2025
@eddeee888 eddeee888 changed the title [resolvers] CODEGEN-500: Support semantic non null [resolvers] CODEGEN-500: Support semanticNonNull custom directive Mar 16, 2025
Copy link
Contributor

github-actions bot commented Mar 16, 2025

🚀 Snapshot Release (alpha)

The latest changes of this PR are available as alpha on npm (based on the declared changesets):

Package Version Info
@graphql-codegen/visitor-plugin-common 5.8.0-alpha-20250320130201-1cf1924a1da9258b15294b0251bba9dc61aa9ad4 npm ↗︎ unpkg ↗︎
@graphql-codegen/typescript-document-nodes 4.0.16-alpha-20250320130201-1cf1924a1da9258b15294b0251bba9dc61aa9ad4 npm ↗︎ unpkg ↗︎
@graphql-codegen/gql-tag-operations 4.0.17-alpha-20250320130201-1cf1924a1da9258b15294b0251bba9dc61aa9ad4 npm ↗︎ unpkg ↗︎
@graphql-codegen/typescript-operations 4.5.2-alpha-20250320130201-1cf1924a1da9258b15294b0251bba9dc61aa9ad4 npm ↗︎ unpkg ↗︎
@graphql-codegen/typescript-resolvers 4.5.0-alpha-20250320130201-1cf1924a1da9258b15294b0251bba9dc61aa9ad4 npm ↗︎ unpkg ↗︎
@graphql-codegen/typed-document-node 5.1.1-alpha-20250320130201-1cf1924a1da9258b15294b0251bba9dc61aa9ad4 npm ↗︎ unpkg ↗︎
@graphql-codegen/typescript 4.1.6-alpha-20250320130201-1cf1924a1da9258b15294b0251bba9dc61aa9ad4 npm ↗︎ unpkg ↗︎
@graphql-codegen/client-preset 4.7.1-alpha-20250320130201-1cf1924a1da9258b15294b0251bba9dc61aa9ad4 npm ↗︎ unpkg ↗︎
@graphql-codegen/graphql-modules-preset 4.0.16-alpha-20250320130201-1cf1924a1da9258b15294b0251bba9dc61aa9ad4 npm ↗︎ unpkg ↗︎

Copy link
Contributor

github-actions bot commented Mar 16, 2025

💻 Website Preview

The latest changes are available as preview in: https://pr-10315.graphql-code-generator.pages.dev

@eddeee888
Copy link
Collaborator Author

Hi @alf, could you please try the following alpha versions:

@graphql-codegen/typescript-resolvers@4.5.0-alpha-20250316110831-3f757aba5365f95cb2ea64e6f2e73cd4ddd534a2

Comment on lines 1527 to 1560
const { mappedTypeKey, resolverType } = ((): { mappedTypeKey: string; resolverType: string } => {
const baseType = getBaseTypeNode(original.type);
const realType = baseType.name.value;
const typeToUse = this.getTypeToUse(realType);
/**
* Turns GraphQL type to TypeScript types (`mappedType`) e.g.
* - String! -> ResolversTypes['String']>
* - String -> Maybe<ResolversTypes['String']>
* - [String] -> Maybe<Array<Maybe<ResolversTypes['String']>>>
* - [String!]! -> Array<ResolversTypes['String']>
*/
const mappedType = this._variablesTransformer.wrapAstTypeWithModifiers(typeToUse, original.type);

const subscriptionType = this._schema.getSubscriptionType();
const isSubscriptionType = subscriptionType && subscriptionType.name === parentName;

if (isSubscriptionType) {
return {
mappedTypeKey: `${mappedType}, "${node.name}"`,
resolverType: 'SubscriptionResolver',
};
}

const directiveMappings =
node.directives
?.map(directive => this._directiveResolverMappings[directive.name as any])
.filter(Boolean)
.reverse() ?? [];

const resolverType = isSubscriptionType ? 'SubscriptionResolver' : directiveMappings[0] ?? 'Resolver';
return {
mappedTypeKey: this.modifyFieldDefinitionNodeTransformedType({ node: original, originalType: mappedType }),
resolverType: directiveMappings[0] ?? 'Resolver',
};
})();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I refactored this bit to bring related variables together, in the same scope - so it's easier to read

@martinbonnin
Copy link

if the type is a list, we make the list non-nullable, each item in the list is not affected

FWIW, the levels parameter allows changing the nullability of items:

type Query {
  a: [String] @semanticNonNull                # List<String?>
  b: [String] @semanticNonNull(levels: [1])   # List<String>?
  c: [String] @semanticNonNull(levels: [0,1]) # List<String>
}

@BenjieGillam
Copy link

Using TypeScript syntax:

type Query {
  a: [String] @semanticNonNull                # Array<String | null>
  b: [String] @semanticNonNull(levels: [1])   # Array<String> | null
  c: [String] @semanticNonNull(levels: [0,1]) # Array<String>
}

You could also use semanticToNullable or semanticToStrict from graphql-sock to convert the schema appropriately, then you'd only need to change a line or three in codegen itself:

https://github.com/graphile/graphql-sock/blob/fdffa2e117fc1ca0fcb5894c4d8d37372b7ccd0f/src/cli.ts#L76-L82

@BenjieGillam
Copy link

BenjieGillam commented Mar 16, 2025

It's also important to note that you should not use a non-nullable type for semantic non-nullable unless the client is an error handling client; i.e. the client must either:

  1. discard entire requests if any errors occur,
  2. throw when an errored field is read from (throw-on-error - see graphql-toe for example)
  3. use @catch or similar directives to influence how errors are handled (but if this is the case, you wouldn't use graphql-codegen since the behavior would change on a per-field basis based on client configuration).

To put it another way: users of graphql-codegen must opt-in to these positions being non-nullable for them; if they don't opt-in then the directive should be ignored and the default nullable should be carried through.

@eddeee888
Copy link
Collaborator Author

Thanks @martinbonnin @BenjieGillam for helping me understand this directive!
I'll look at the shared resources and update the PR soon 👍

It's also important to note that you should not use a non-nullable type for semantic non-nullable unless the client is an error handling client

Did you mean something like this?

type Foo {
  bar: String! @semanticNonNull
}

To put it another way: users of graphql-codegen must opt-in to these positions being non-nullable for them; if they don't opt-in then the directive should be ignored and the default nullable should be carried through.

This sounds like a behaviour for the client part of codegen if I understand it correctly? If so, I'll keep that in mind when implementing the client part

@benjie
Copy link
Contributor

benjie commented Mar 17, 2025

I meant that

type Query {
  bar: String @semanticNonNull
}

With query

query Q {
  bar
}

Should come out as:

type Q = {
  bar: string | null
}

unless the user opts in to having it come out as non-nullable (e.g. via a configuration parameter, choice of preset, etc), at which point you'd generate:

type Q = {
  bar: string
}

This is because query Q would always return a bar field, and it can be null if an error occurs, e.g.:

{
  "data": { "bar": null },
  "errors": [{"message": "Blah blah blah", "path": ["bar"]}]
}

e.g. strictly the field is still string | null. It's only when the client prevents a user from reading that null where you know it can be string alone - e.g. if you have throw-on-error then you can never read that null (since an error will be returned if you attempt it) and thus the only value you can read from it is string.

If you're doing pure type generation then you have no control over this. If you're doing code generation too, then you can - you can choose to throw the errors either when the user reads from the field that's broken (e.g. using a "getter" as we do in graphql-toe) or, iff you have fragment isolation (e.g. via masking), you could do it when the user uses the fragment if the non-masked part of the fragment contain an error. In this latter case though, you would have to ensure that there's no way around this, otherwise the user could still read a null from this position.

Personally I'd recommend using graphql-toe, it ensures that no matter how you read the data (once it has gone through TOE) whenever you read an errored field, an error will be thrown. I also put quite a bit of effort into ensuring it was as efficient as possible, only creating new types/adding getters where it had to, and carrying through the source data otherwise.

@martinbonnin
Copy link

martinbonnin commented Mar 17, 2025

bar: String @semanticNonNull should come out as:

type Q = {
  bar: string | null
}

unless the user opts in to having it come out as non-nullable

Disagree on this one. If someone takes the time to annotate their schema with @semanticNonNull, I would expect codegen to generate the field as non-null by default. It's on the user to do the correct runtime thing (by using graphql-toe or other).

If the client is not an error handling client, I'm fine with an option to ignore @semanticNonNull altogether but if we want to raise awareness around this, I'd rather have graphql-code-generator default to the new "error-handling" mode.

@benjie
Copy link
Contributor

benjie commented Mar 17, 2025

Strong disagree; the team producing the schema (e.g. adding @semanticNonNull to it) and the team consuming the schema (e.g. the client frontend team) are often different. Over time we'd like to move most people over to using error handling clients, but right now the vast majority of clients are not "error handling clients" (i.e. they don't have "throw on error" or similar behavior preventing the user from reading an "error null").

If we were to generate non-nulls from semantic non-null right now, then the types would be a lie - e.g. someone using a semantic non-null marked schema with Apollo Client would be able to read null from positions that the types say are non-nullable, which could produce unexpected TypeError, or worse: subtle rendering or behavior bugs... e.g. proliferation of NaNs.

If it's the client team adding @semanticNonNull to their schema, then they should already know to configure codegen into "error handling" mode, it's just one extra line of config for them.

@benjie
Copy link
Contributor

benjie commented Mar 17, 2025

@eddeee888 I've just pushed out a new version of graphql-sock with better documentation, including documenting the library mode. I think you'd just need to add a line wherever you build/source the GraphQL schema:

import { semanticToStrict, semanticToNullable } from 'graphql-sock';

//...

const schema =
  codegenOptions.errorHandlingClient
    ? semanticToStrict(sourceSchema)
    : semanticToNullable(sourceSchema);

Note that graphql-sock supports both the @semanticNonNull directive and the GraphQLSemanticNonNull schema type, which means it supports all proposed solutions to the semantic nullability problem; though you'd need to be running the relevant version of graphql.js with the syntax support for the solution you're using (except solutions 5 and 6 which will always work). E.g. to support the syntax for solution 1, you'd need to be running graphql@canary-pr-4192.

@eddeee888
Copy link
Collaborator Author

eddeee888 commented Mar 18, 2025

Hi @benjie,
Thanks for the update 👍 I like the approach of updating the schema, which is more accurate than updating arbitrary string, like what we are doing in this PR.

Some request I have with this current version:

  • It's using the canary version of GraphQL for the GraphQLSemanticNonNull check. It's going to be hard for consumers, including Codegen, to use as most would have a different version installed. e.g. When I try to use in Codegen repo, it'd say GraphQLSemanticNonNull doesn't exist. Do you have advices here?
  • Is it possible to call a function to update one field node, instead of the whole schema?

@benjie
Copy link
Contributor

benjie commented Mar 18, 2025

Good catch; I forgot that was why I only had CLI docs and not library docs! Should be feasible for it to import * as graphql from 'graphql' and use GraphQLSemanticNonNull iff it exists - let me get back to you on that 👍

@benjie
Copy link
Contributor

benjie commented Mar 19, 2025

@eddeee888 I've released [email protected] which is now tested against v15, v16, v17 and graphql@canary-pr-4192 in CI; it also splits the library code from the CLI code so there's no imports of anything but graphql in library mode, so should be safe to bundle and run on the web too.

Oh, also it now exports a convertFieldConfig method to just convert an individual GraphQLFieldConfig rather than the entire schema 👍

testingSchema,
[],
{
customDirectives: { semanticNonNull: true },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that semanticNonNull is not necessarily a directive - it can also be represented by syntax (and because you're using graphql-sock the syntax should automatically work).

Further, fields can be marked as semantically non-nullable in the schema without the client having any handling code, in which case the user can still read null from those positions (on error), and thus the generated code saying it's non-nullable would be a lie.

Instead, consider renaming the option to something that indicates that the client is handling errors such that you can never read a null from a position that is linked to an error:

Suggested change
customDirectives: { semanticNonNull: true },
noErrorNulls: true,

Essentially:

  • if you use graphql-toe or similar then you can set noErrorNulls: true and GraphQL Codegen can output non-nullable in positions that are semantically non-nullable
  • if you don't have any specific client handling code, then Codegen should output these same positions as nullable, since they will be null if an error occurs

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is the PR for typescript-resolvers, we should look at this from the server's POV.

Note that semanticNonNull is not necessarily a directive - it can also be represented by syntax (and because you're using graphql-sock the syntax should automatically work).

I feel there could a future where the syntax can be handled out-of-the-box by codegen i.e. it is possible for codegen to treat the semanticNonNull syntax (* in option 1 IIRC?) the same as !. When that happens, users can stop using this config option.

Further, fields can be marked as semantically non-nullable in the schema without the client having any handling code, in which case the user can still read null from those positions (on error), and thus the generated code saying it's non-nullable would be a lie.

In this PR, the types are for resolver return types. In my mind, these resolvers will get TypeScript error when returning null. They must throw for the value to be null, in which case clients should handle accordingly.
Unless I misunderstood the server's responsibilities when handling semanticNonNull? 🤔

noErrorNulls: true,

I feel something like noErrorNulls on clients makes sense, and each client may choose to handle this differently. However, the server shouldn't change how it returns non-nullable data, and throw error to force null data.

Again, I could be misunderstanding the server's role here. Keen to hear advices if so 🙂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope this is entirely my mistake; I missed this was for the server side and thought it was client side. Server side, semantic non-null and traditional non-null are equivalent: a null must not be returned. I personally wouldn’t have a setting server-side, just always enable it if either the @semanticNonNull directive is used or the syntax is used (i.e. just feed the source schema through semanticToStrict unconditionally); but you should do whatever is right for the library.

@eddeee888 eddeee888 merged commit f6909d1 into master Mar 21, 2025
18 of 19 checks passed
@eddeee888 eddeee888 deleted the support-semantic-non-null branch March 21, 2025 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants