Skip to content

[operations][client-preset] CODEGEN-500 - Support semantic non-null for clients#10323

Merged
eddeee888 merged 11 commits intomasterfrom
support-semantic-non-null-client
Mar 26, 2025
Merged

[operations][client-preset] CODEGEN-500 - Support semantic non-null for clients#10323
eddeee888 merged 11 commits intomasterfrom
support-semantic-non-null-client

Conversation

@eddeee888
Copy link
Copy Markdown
Collaborator

@eddeee888 eddeee888 commented Mar 21, 2025

Description

This PR supports @semanticNonNull support for client use cases:

  • typescript-operations
  • client-preset

Introducing semanticNonNull config option. This will be the target option for all semantic non null options in the future.
For now, it includes an option called errorHandlingClient so users can declare whether they are using an error-handling client or not:

  • If an error handling client is used, all semantic non-null fields are treated as non-null because accessing these values will result in a thrown error
  • If an error handling client is not used, all semantic non-null fields are treated as null, which means when a value is null, it could be "data null" or "error null"

Type of change

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

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Unit test

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Mar 21, 2025

🦋 Changeset detected

Latest commit: 739ada6

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

This PR includes changesets to release 2 packages
Name Type
@graphql-codegen/typescript-operations Minor
@graphql-codegen/client-preset Minor

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

Comment on lines +330 to +332
semanticNonNull?: {
errorHandlingClient: boolean;
};
Copy link
Copy Markdown
Collaborator Author

@eddeee888 eddeee888 Mar 21, 2025

Choose a reason for hiding this comment

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

I'm thinking this object can be extended to handle other related semanticNonNull functionalities e.g. @catch?
(I'm not entirely sure how it'd work yet, but I imagine there could be options we turn on/off here 😅 )

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I see @catch as mainly orthogonal to @semanticNonNull. Sure they make a lot of sense together but you can have an error handling client that uses @catch(to: RESULT) or @catch(to: THROW) without semantic nullability on the server.

That client will not get the semantic nullability information but could still use @catch to modify the shape of the generated code.

I would keep it simple/yagni with just a plain Boolean option:

semanticNonNull?: boolean

If really you want to bundle nullability options together, maybe something like so:

nullability?: {
  semanticNonNull: boolean;
  catch: boolean;
}

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Mar 21, 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-20250326083144-739ada6d1bd51f5f1a505e18b205b099c796aad3 npm ↗︎ unpkg ↗︎
@graphql-codegen/typescript-document-nodes 4.0.16-alpha-20250326083144-739ada6d1bd51f5f1a505e18b205b099c796aad3 npm ↗︎ unpkg ↗︎
@graphql-codegen/gql-tag-operations 4.0.17-alpha-20250326083144-739ada6d1bd51f5f1a505e18b205b099c796aad3 npm ↗︎ unpkg ↗︎
@graphql-codegen/typescript-operations 4.6.0-alpha-20250326083144-739ada6d1bd51f5f1a505e18b205b099c796aad3 npm ↗︎ unpkg ↗︎
@graphql-codegen/typescript-resolvers 4.5.0-alpha-20250326083144-739ada6d1bd51f5f1a505e18b205b099c796aad3 npm ↗︎ unpkg ↗︎
@graphql-codegen/typed-document-node 5.1.1-alpha-20250326083144-739ada6d1bd51f5f1a505e18b205b099c796aad3 npm ↗︎ unpkg ↗︎
@graphql-codegen/typescript 4.1.6-alpha-20250326083144-739ada6d1bd51f5f1a505e18b205b099c796aad3 npm ↗︎ unpkg ↗︎
@graphql-codegen/client-preset 4.8.0-alpha-20250326083144-739ada6d1bd51f5f1a505e18b205b099c796aad3 npm ↗︎ unpkg ↗︎
@graphql-codegen/graphql-modules-preset 4.0.16-alpha-20250326083144-739ada6d1bd51f5f1a505e18b205b099c796aad3 npm ↗︎ unpkg ↗︎

Comment thread packages/presets/client/src/index.ts Outdated
const isPersistedOperations = !!options.presetConfig?.persistedDocuments;
if (options.config.semanticNonNull?.errorHandlingClient) {
options.schemaAst = await semanticToStrict(options.schemaAst!);
options.schema = options.schemaAst as any; // FIXME: `schemaAst` _seems_ to be able to used as `schema`, but not sure if it has any side effects?
Copy link
Copy Markdown
Collaborator Author

@eddeee888 eddeee888 Mar 24, 2025

Choose a reason for hiding this comment

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

I'm a bit unsure about this:

  • schema is a DocumentNode
  • shemaAst is a GraphQLSchema
  • We can forcefully override schema with schemaAst ... and it works 🤔

Maybe when loading schema in each generates block, we do a check? That's my best guess though, and if anyone has more context, please let me know!

EDIT: Looks like it's somewhere here 👀

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe what you want here is this?

Suggested change
options.schema = options.schemaAst as any; // FIXME: `schemaAst` _seems_ to be able to used as `schema`, but not sure if it has any side effects?
options.schema = parse(printSchema(options.schemaAst));

Just guessing from the types you mention, I'm not familiar with this codebase.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Mar 24, 2025

💻 Website Preview

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

@eddeee888
Copy link
Copy Markdown
Collaborator Author

eddeee888 commented Mar 24, 2025

Hi @benjie @martinbonnin, this PR is the client type implementation semanticNonNull, I've left how it works in the PR description.

Copy link
Copy Markdown
Contributor

@benjie benjie left a comment

Choose a reason for hiding this comment

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

LGTM! I've left a few suggestions, but really excited to see this get merged! 🚀

* export default config;
* ```
*/
semanticNonNull?: {
Copy link
Copy Markdown
Contributor

@benjie benjie Mar 24, 2025

Choose a reason for hiding this comment

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

Perhaps, and this is just a suggestion, rename this to "semantic nullability" as a more generic term. If we were to accept a solution that used a document directive to enable the syntax, this is the likely name that we would use for that directive, and it doesn't tie the solution to one specific implementation. (GraphQL-SOCK can be updated to work with other solutions without any effort on your part.)

Suggested change
semanticNonNull?: {
semanticNullability?: {

Comment on lines +10 to +14
schema: GraphQLSchema,
rawDocuments: Types.DocumentFile[],
config: TypeScriptDocumentsPluginConfig
) => {
const transformedSchema = config.semanticNonNull?.errorHandlingClient ? await semanticToStrict(schema) : schema;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I personally would rename the schema variable to inputSchema so that schema, the variable you'd use by default, is the correct variable to use:

Suggested change
schema: GraphQLSchema,
rawDocuments: Types.DocumentFile[],
config: TypeScriptDocumentsPluginConfig
) => {
const transformedSchema = config.semanticNonNull?.errorHandlingClient ? await semanticToStrict(schema) : schema;
inputSchema: GraphQLSchema,
rawDocuments: Types.DocumentFile[],
config: TypeScriptDocumentsPluginConfig
) => {
const schema = config.semanticNonNull?.errorHandlingClient ? await semanticToStrict(inputSchema) : inputSchema;

(This would also mean no other changes were needed in the function.)

return sock.semanticToStrict(schema);
} catch {
throw new Error(
"To use the `customDirective.semanticNonNull` option, you must install the 'graphql-sock' package."
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
"To use the `customDirective.semanticNonNull` option, you must install the 'graphql-sock' package."
"To use the `semanticNonNull.errorHandlingClient` option, you must install the 'graphql-sock' package."

Comment thread packages/presets/client/src/index.ts Outdated
const isPersistedOperations = !!options.presetConfig?.persistedDocuments;
if (options.config.semanticNonNull?.errorHandlingClient) {
options.schemaAst = await semanticToStrict(options.schemaAst!);
options.schema = options.schemaAst as any; // FIXME: `schemaAst` _seems_ to be able to used as `schema`, but not sure if it has any side effects?
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe what you want here is this?

Suggested change
options.schema = options.schemaAst as any; // FIXME: `schemaAst` _seems_ to be able to used as `schema`, but not sure if it has any side effects?
options.schema = parse(printSchema(options.schemaAst));

Just guessing from the types you mention, I'm not familiar with this codebase.

- [`nonOptionalTypename`](/plugins/typescript/typescript#nonoptionaltypename): Automatically adds `__typename` field to the generated types, even when they are not specified in the selection set, and makes it non-optional.
- [`avoidOptionals`](/plugins/typescript/typescript#avoidoptionals): This will cause the generator to avoid using TypeScript optionals (`?`) on types.
- [`documentMode`](#documentmode): Allows you to control how the documents are generated.
- [`semanticNonNull`](/plugins/typescript/typescript-operations#semanticnonnull): Handles `@semanticNonNull` directive based on client-side requirements.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
- [`semanticNonNull`](/plugins/typescript/typescript-operations#semanticnonnull): Handles `@semanticNonNull` directive based on client-side requirements.
- [`semanticNullability`](/plugins/typescript/typescript-operations#semanticnullability): Indicate the client capabilities to get stronger types with [semantic nullability](https://github.com/graphql/graphql-wg/blob/main/rfcs/SemanticNullability.md)-enabled schemas.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I like semanticNonNull. This option is part of the overall solution but doesn't solve all of semantic nullability. It's very specific about the codegen and the support of @semanticNonNull but doesn't touch the runtime.

Counter proposal:

Suggested change
- [`semanticNonNull`](/plugins/typescript/typescript-operations#semanticnonnull): Handles `@semanticNonNull` directive based on client-side requirements.
- [`semanticNonNull`](/plugins/typescript/typescript-operations#semanticnonnull): fields marked as semantically non null using the [`@semanticNonNull` directive](https://specs.apollo.dev/nullability/v0.2/#@semanticNonNull)
are generated as non-null in TypeScript.
Enabling this config requires an error-handling client. This can for an example be done using [graphql-toe](https://github.com/graphile/graphql-toe).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

By using graphql-sock this supports not only @semanticNonNull the directive but all the proposed solutions (assuming GraphQL.js support) to the semantic nullability RFC. Theoretically this should already work just fine with the * syntax when using graphql@canary-pr-4192 for example (I've not tested it though). If we were to come up with an alternative solution (e.g. GraphQLStrictNonNull to represent the old "kills parent on exception" style of non-null) then "semantic nullability" would still cover this solution, but "semanticNonNull" would not.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd support renaming the option to simply nullability though, as you proposed separately.

Copy link
Copy Markdown

@martinbonnin martinbonnin left a comment

Choose a reason for hiding this comment

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

Looks great!! A couple documentation/terminology comments but otherwise looking forward to play with this 🚀 !!

Comment on lines +330 to +332
semanticNonNull?: {
errorHandlingClient: boolean;
};
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I see @catch as mainly orthogonal to @semanticNonNull. Sure they make a lot of sense together but you can have an error handling client that uses @catch(to: RESULT) or @catch(to: THROW) without semantic nullability on the server.

That client will not get the semantic nullability information but could still use @catch to modify the shape of the generated code.

I would keep it simple/yagni with just a plain Boolean option:

semanticNonNull?: boolean

If really you want to bundle nullability options together, maybe something like so:

nullability?: {
  semanticNonNull: boolean;
  catch: boolean;
}

- [`nonOptionalTypename`](/plugins/typescript/typescript#nonoptionaltypename): Automatically adds `__typename` field to the generated types, even when they are not specified in the selection set, and makes it non-optional.
- [`avoidOptionals`](/plugins/typescript/typescript#avoidoptionals): This will cause the generator to avoid using TypeScript optionals (`?`) on types.
- [`documentMode`](#documentmode): Allows you to control how the documents are generated.
- [`semanticNonNull`](/plugins/typescript/typescript-operations#semanticnonnull): Handles `@semanticNonNull` directive based on client-side requirements.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I like semanticNonNull. This option is part of the overall solution but doesn't solve all of semantic nullability. It's very specific about the codegen and the support of @semanticNonNull but doesn't touch the runtime.

Counter proposal:

Suggested change
- [`semanticNonNull`](/plugins/typescript/typescript-operations#semanticnonnull): Handles `@semanticNonNull` directive based on client-side requirements.
- [`semanticNonNull`](/plugins/typescript/typescript-operations#semanticnonnull): fields marked as semantically non null using the [`@semanticNonNull` directive](https://specs.apollo.dev/nullability/v0.2/#@semanticNonNull)
are generated as non-null in TypeScript.
Enabling this config requires an error-handling client. This can for an example be done using [graphql-toe](https://github.com/graphile/graphql-toe).

@eddeee888
Copy link
Copy Markdown
Collaborator Author

eddeee888 commented Mar 26, 2025

Thanks @benjie @martinbonnin for your great suggestions!

I've manually applied the suggestions because the changes need to happen across multiple files:

  • Renamed the config to nullability.errorHandlingClient: nullability proposal makes a lot of sense here, and it can be extended future options that interact with nullabilities
  • Minor code logic based on suggestions

@eddeee888
Copy link
Copy Markdown
Collaborator Author

Merging this so we can try out the alpha versions! We can make changes before the final release if needed 🙂

@eddeee888 eddeee888 merged commit f3cf4df into master Mar 26, 2025
19 checks passed
@eddeee888 eddeee888 deleted the support-semantic-non-null-client branch March 26, 2025 08:42
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.

3 participants