[visitor-plugin-common] fix: satisfy strict mode in selection-set-to-object, client-side-base-visitor and base-documents-visitor (#4683) by eddeee888 · Pull Request #11031 · dotansimha/graphql-code-generator

eddeee888 · 2026-10-03T14:27:16Z

Description

This PR is part 2 of 3 for visitor-plugin-common in the strict-mode tracker #10985. Once the root tsconfig enables strict: true, selection-set-to-object.ts has 31 errors, client-side-base-visitor.ts has 17 and base-documents-visitor.ts has 5. This PR fixes all 53. Where null or undefined really happens, it widens the shared type. Everywhere else it adds a guard, a fallback, or a throw with a clear message. It adds no new as casts or ! assertions. base-types-visitor, utils and the remaining files are left for part 3.

Main

BaseDocumentsVisitor._selectionSetToObject is optional, because only setSelectionSetHandler sets it. FragmentDefinition and OperationDefinition throw a clear error when it isn't set. FragmentDefinition also throws one when the fragment's type condition isn't in the schema.
BaseDocumentsVisitor.FragmentDefinition and ClientSideBaseVisitor.buildOperation return string | null. Both already return null at runtime.
SelectionSetToObject.transformSelectionSet declares its return type and throws when the instance has no selection set. flattenSelectionSet throws when it has no parent type.
LinkField.alias is optional. BaseSelectionSetProcessor.typeCache is keyed by Location | undefined, because documents parsed with noLocation have no loc.
getPossibleTypes takes GraphQLType | undefined, which drops its as GraphQLNamedType cast. It now returns a copy of the schema's readonly possible-types array instead of casting it. OperationVariablesToObject.transform accepts undefined. Both already handled these inputs at runtime.
transformSelectionSet filters its results with type guards, which replaces as string[] and the annotated .map((t: NameAndType) => ...).
In buildSelectionSet, the link-field map keeps the merged selectionSet next to the original field, instead of copying field with a merged selectionSet into it. Alias fields take their alias from the map key.
ClientSideBaseVisitor defaults documents to []. It only calls _onExecutableDocumentNode when that callback is set. _findDeferredFields walks its queue with for...of, which also visits the selections pushed while iterating, instead of shift().

The new throws are the only runtime changes. Each one replaces a path that already failed with a TypeError.

Related #4683

Type of change

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Typecheck with master's tsconfig: 0 errors
Typecheck with chore: enable strict: true in root tsconfig (#4683) #10985's strict tsconfig: errors went from 127 to 73. None are left in the three files, and every other file's count is the same or lower.
Unit tests for visitor-plugin-common, typescript, typescript-operations, typed-document-node and client-preset: all pass, no snapshot changes
Lint and format

Further comments

selectionSetStringFromFields returns null when there are no fields, for example when only __typename is selected. The grouping in _buildGroupedSelections used that null as an object key, so it became "null". This PR keeps that key with String(...) and a comment, so generated output doesn't change.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Adze75p1tHBWWaHdc4fpXk

Generated by Claude Code

changeset-bot · 2026-10-03T14:27:22Z

🦋 Changeset detected

Latest commit: 37e83a2

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

This PR includes changesets to release 1 package

Name	Type
@graphql-codegen/visitor-plugin-common	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

github-actions · 2026-10-03T14:28:39Z

🚀 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/cli`	`7.4.4-alpha-20261005142022-37e83a27ce324092c7e2beb909dccd1fc2123910`	npm ↗︎ unpkg ↗︎
`@graphql-codegen/time`	`7.1.1-alpha-20261005142022-37e83a27ce324092c7e2beb909dccd1fc2123910`	npm ↗︎ unpkg ↗︎
`@graphql-codegen/visitor-plugin-common`	`7.2.8-alpha-20261005142022-37e83a27ce324092c7e2beb909dccd1fc2123910`	npm ↗︎ unpkg ↗︎
`@graphql-codegen/typescript-operations`	`6.1.10-alpha-20261005142022-37e83a27ce324092c7e2beb909dccd1fc2123910`	npm ↗︎ unpkg ↗︎
`@graphql-codegen/typescript`	`6.1.1-alpha-20261005142022-37e83a27ce324092c7e2beb909dccd1fc2123910`	npm ↗︎ unpkg ↗︎
`@graphql-codegen/client-preset`	`6.2.1-alpha-20261005142022-37e83a27ce324092c7e2beb909dccd1fc2123910`	npm ↗︎ unpkg ↗︎
`@graphql-codegen/plugin-helpers`	`7.4.1-alpha-20261005142022-37e83a27ce324092c7e2beb909dccd1fc2123910`	npm ↗︎ unpkg ↗︎

eddeee888

Generated by Claude Code

eddeee888 · 2026-10-03T15:40:26Z

            // in case the possible type actually implements the given interface
-            if (isTypeSubTypeOf(this._schema, possibleType, fragmentSpreadType)) {
+            if (
+              fragmentSpreadType &&


Note: getType returns undefined for a fragment on a type the schema doesn't have. In that case the spread is skipped, like a type it doesn't match, instead of throwing here. Document validation normally rejects that fragment before codegen gets this far.

Generated by Claude Code

Generated by Claude Code

eddeee888 · 2026-10-03T15:40:28Z

            spread.name.value,
            fragmentSuffix,
-            possibleTypesForFragment.length === 1 ? null : possibleType.name,
+            possibleTypesForFragment.length === 1 ? undefined : possibleType.name,


Note: buildFragmentTypeName's third param is already optional, so passing undefined avoids widening it to also accept null.

Generated by Claude Code

Generated by Claude Code

eddeee888 · 2026-10-03T15:40:32Z

+    let typeNodes = types.get(typeName);
+    if (!typeNodes) {
+      typeNodes = [];
+      types.set(typeName, typeNodes);


Note: The list is read once into a local, so the push below doesn't need types.get(typeName) to be non-null. Calling has then get doesn't narrow the type.

Generated by Claude Code

Generated by Claude Code

eddeee888 · 2026-10-03T15:40:36Z

                    parentFieldName: parentName,
                  });
-                prev[typeName].push(this.selectionSetStringFromFields(partialFields));
+                prev[typeName].push(this.selectionSetStringFromFields(partialFields) || '');


Note: selectionSetStringFromFields returns null when there are no fields. Every reader of these grouped strings filters with .filter(Boolean), or joins the union array where '' and null print the same. So the fallback is '' and GroupedStringifiedTypes doesn't have to accept null. The same applies at L663, L685, L765, L784 and L801.

Generated by Claude Code

Generated by Claude Code

eddeee888 · 2026-10-03T15:40:39Z

+    const transformed = [
+      ...(transformedTypenameFields || []),
+      ...(transformedPrimitiveFields || []),
+      ...(transformedAliasesPrimitiveFields || []),
+      ...(transformedLinkFields || []),


Note: ProcessResult is null | Array<…>, and processors can return null, so || [] is real handling. The old ProcessResult annotation on this array hid that, because spreading null here would have thrown.

Generated by Claude Code

Generated by Claude Code

eddeee888 · 2026-10-03T15:40:42Z

      meta = this._onExecutableDocumentNode({
        kind: Kind.DOCUMENT,
-        definitions,
+        definitions: definitions || [],


Note: definitions is an optional param of this protected method, so a subclass that doesn't pass it gets an empty document here instead of one with definitions: undefined.

Generated by Claude Code

Generated by Claude Code

eddeee888 · 2026-10-03T15:40:46Z

            this._imports.add(
              `import * as Operations from '${this.clearExtension(
-                this.config.importDocumentNodeExternallyFrom,
+                this.config.importDocumentNodeExternallyFrom || '',


Note: This branch is reached right after the warning above when importDocumentNodeExternallyFrom isn't set. With || '', codegen carries on past the warning and emits from '', where before extname(undefined) threw. That is a small runtime change. Same for location || '' at L678, for a document with no location.

Generated by Claude Code

Generated by Claude Code

eddeee888 · 2026-10-03T15:40:49Z

              ...fragmentImports[0].importSource,
              identifiers: unique(
-                flatten(fragmentImports.map(fi => fi.importSource.identifiers)),
+                flatten(fragmentImports.map(fi => fi.importSource.identifiers || [])),


Note: ImportSource.identifiers is optional, so a fragment import without identifiers adds none to the merged import instead of making flatten fail on undefined.

Generated by Claude Code

Generated by Claude Code

…object, client-side-base-visitor and base-documents-visitor (#4683) BaseDocumentsVisitor._selectionSetToObject is optional and FragmentDefinition returns `string | null`; ClientSideBaseVisitor.buildOperation returns `string | null`. LinkField's `alias` is optional, BaseSelectionSetProcessor.typeCache is keyed by `Location | undefined`, and getPossibleTypes / OperationVariablesToObject.transform accept `undefined`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Adze75p1tHBWWaHdc4fpXk eddeee888:oss:issue-fix

…hed code `transformSelectionSet` filters its results with type guards instead of `as string[]` and an annotated `.map` param. `getPossibleTypes` takes `GraphQLType | undefined`, so unwrapping lists and non-nulls needs no cast, and copies the schema's readonly possible-types array. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Adze75p1tHBWWaHdc4fpXk

eddeee888 · 2026-10-05T14:18:23Z

 };
 export type LinkField = {
-  alias: string;
+  alias?: string;


Why is this optional now?

Link fields have always been built with alias: undefined when the field has no alias (selection-set-to-object.ts L972-977: `alias: field.alias ? formatNamedField(...) : undefined`). Without `strictNullChecks` that still matched `alias: string`. With it turned on, that object fails TS2322. The processors that read `LinkField` already treat the alias as optional, using `field.alias || field.name` ([`pre-resolve-types.ts` L125](https://github.com/dotansimha/graphql-code-generator/blob/37e83a27ce324092c7e2beb909dccd1fc2123910/packages/plugins/other/visitor-plugin-common/src/selection-set-processor/pre-resolve-types.ts#L125)`` and ts-selection-set-processor.ts L85),`` so alias?: string makes the type match what the code already does.

Generated by Claude Code

Generated by Claude Code

eddeee888 · 2026-10-05T14:27:09Z

 export function getPossibleTypes(
  schema: GraphQLSchema,
-  type: GraphQLNamedType,
+  type: GraphQLType | undefined,


Why is this undefined now?

Two kinds of callers pass undefined here:

SelectionSetToObject passes this._parentSchemaType (L510, [L1132](https://github.com/dotansimha/graphql-code-generator/blob/37e83a27ce324092c7e2beb909dccd1fc2123910/packages/plugins/other/visitor-plugin-common/src/selection-set-to-object.ts#L1132)),`` and that is an optional constructor param (L126).``

The fragment-spread path passes the result of schema.getType(...) (L316-317),`` which is GraphQLNamedType | undefined.

The function already handles undefined: none of the isListType/isObjectType/isAbstractType checks match it, so it falls through to return []. Widening the param removes those TS2345 errors without adding a guard at each call site. Widening it to GraphQLType rather than GraphQLNamedType also lets the recursive call pass type.ofType without the old as GraphQLNamedType cast.

Generated by Claude Code

Generated by Claude Code

eddeee888 · 2026-10-05T14:27:43Z


  transform<TDefinitionType extends InterfaceOrVariable>(
-    variablesNode: ReadonlyArray<TDefinitionType>,
+    variablesNode: ReadonlyArray<TDefinitionType> | undefined,


Why has | undefined now?

In graphql-js, variableDefinitions is optional on OperationDefinitionNode and FragmentDefinitionNode (readonly variableDefinitions?: ReadonlyArray<VariableDefinitionNode> | undefined, graphql-js ast.d.ts). BaseDocumentsVisitor passes node.variableDefinitions straight in (L509, [L540-542](https://github.com/dotansimha/graphql-code-generator/blob/37e83a27ce324092c7e2beb909dccd1fc2123910/packages/plugins/other/visitor-plugin-common/src/base-documents-visitor.ts#L540-L542)),`` which is 2 of this PR's TS2345 errors in base-documents-visitor.ts. transform already handles undefined on its first line, if (!variablesNode || variablesNode.length === 0) return null; (L53-55),`` so widening the param documents existing behaviour instead of adding || [] at every caller.

Generated by Claude Code

Generated by Claude Code

eddeee888 mentioned this pull request Oct 3, 2026

chore: enable strict: true in root tsconfig (#4683) #10985

Merged

24 of 25 tasks

eddeee888 commented Oct 3, 2026

View reviewed changes

claude added 2 commits October 5, 2026 14:18

eddeee888 commented Oct 5, 2026

View reviewed changes

eddeee888 force-pushed the claude/trusting-dirac-qj39cq branch from 23a96c8 to 37e83a2 Compare October 5, 2026 14:19

eddeee888 commented Oct 5, 2026

View reviewed changes

eddeee888 marked this pull request as ready for review October 5, 2026 14:36

eddeee888 merged commit 3caccd3 into master Oct 5, 2026
22 checks passed

eddeee888 deleted the claude/trusting-dirac-qj39cq branch October 5, 2026 14:41

github-actions Bot mentioned this pull request Oct 5, 2026

Upcoming Release Changes #11008

Merged

Conversation

eddeee888 commented Oct 3, 2026

Description

Main

Type of change

How Has This Been Tested?

Further comments

Uh oh!

changeset-bot Bot commented Oct 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions Bot commented Oct 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🚀 Snapshot Release (alpha)

Uh oh!

eddeee888 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

changeset-bot Bot commented Oct 3, 2026 •

edited

Loading

github-actions Bot commented Oct 3, 2026 •

edited

Loading

🚀 Snapshot Release (`alpha`)