[visitor-plugin-common] fix: satisfy strict mode in base-resolvers-visitor (#4683) by eddeee888 · Pull Request #11030 · dotansimha/graphql-code-generator

eddeee888 · 2026-10-03T12:46:45Z

Description

This PR is part 1 of 3 for visitor-plugin-common in the strict-mode tracker #10985. It fixes all 46 errors that base-resolvers-visitor.ts hits once the root tsconfig enables strict: true, by widening shared types and giving nullable values fallbacks or type guards, with no new as casts or ! assertions. The other files in the package are left for parts 2 and 3.

I split visitor-plugin-common into 3 PRs because it holds 173 of the remaining errors, which is too many to review in one pass.

Main

BaseResolversVisitor's constructor takes additionalConfig as Partial<TPluginConfig>, since the visitor fills in a default for every key that's left out. This clears the 20 TS2783 errors (keys set before a spread that overwrites them) and drops the as TPluginConfig cast.
ScalarTypeDefinition, DirectiveDefinition and EnumTypeDefinition return string | null, matching the null they already return for skipped federation scalars and directives, and for enums without mappers or enumValues.
getAbstractMembersType's isTypenameNonOptional is optional.
defaultMapperType throws a descriptive error when defaultMapper isn't set, instead of failing with a TypeError. This is the one runtime change. It only affects code paths that already crashed.
Field arguments, field directives and union types fall back to []. These fallbacks have no "only to satisfy type safety" comment, because BaseResolversVisitor is exported, so subclasses visiting other ASTs can pass nodes without them: the fallback is real handling. 51ca87b removes the comments this PR first added there.
as ExternalParsedMapper is gone: the mappers are filtered by isExternal. The casts that remain (node.type as any as string, node.fields as unknown as FieldDefinitionResult[], parent: any, parent[key] as UnionTypeDefinitionNode, parent[key] as DirectiveDefinitionNode) each have a comment saying why they're needed and what removing them would take.

Drive-by

9d603dc removes the same kind of "only to satisfy type safety" comment from two existing real-handling defaults, for the same reason: TsVisitor (typescript/src/visitor.ts, originalNode.types || []) is exported and a parsed union can have no types, and CodegenContext (cli/src/config.ts, generates: {}) is exported and can be created without config. Comment-only, so no changeset for typescript or cli.

Related #4683

Type of change

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

How Has This Been Tested?

Typecheck on master's tsconfig: 0 errors
Typecheck with chore: enable strict: true in root tsconfig (#4683) #10985's strict tsconfig: 173 → 127 errors, none of them in base-resolvers-visitor.ts, and every other file's count unchanged
Unit tests for visitor-plugin-common and typescript-resolvers, with no snapshot changes
Lint and format

Further comments

On master, an enum mapped by an object in enumValues already generates MyEnum: null; in ResolversTypes, because its sourceIdentifier is null. This PR keeps that output the same (String(sourceIdentifier), with a comment) so generated code doesn't change. Fixing that output is out of scope here.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VazhaQJ1EYc8Mk39FnQxJ6

changeset-bot · 2026-10-03T12:46:49Z

🦋 Changeset detected

Latest commit: 9d603dc

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-03T12:48:05Z

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

eddeee888 · 2026-10-03T13:11:00Z

      const isMapped = this.config.mappers[typeName];
      const isScalar = this.config.scalars[typeName];
-      const hasDefaultMapper = !!this.config.defaultMapper?.type;
+      const defaultMapperType = this.config.defaultMapper?.type;


Note: I keep the value in a local here instead of the old hasDefaultMapper boolean. TypeScript doesn't narrow this.config.defaultMapper from a boolean computed earlier, but defaultMapperType && narrows the local to string, so the later reads need no !. The same applies at L1197, and to mappedValues in EnumTypeDefinition.

Generated by Claude Code

eddeee888 · 2026-10-03T13:11:03Z

    }

-    return this.convertName(node, null, true);
+    return this.convertName(node, {}, true);


Note: options is optional and doesn't accept null, so {} replaces it. The output is the same: convertName and the naming convert functions read every option with ?., and {} is the convert functions' own default.

Generated by Claude Code

eddeee888 · 2026-10-03T13:11:05Z


        if (this._federation.isResolveReferenceField(node)) {
-          if (!this._federation.getMeta()[parentType.name].hasResolveReference) {
+          if (!this._federation.getMeta()[parentName].hasResolveReference) {


Note: I key this by parentName and dropped the this.schema.getType(parentName) lookup. parentType.name was always parentName, and the lookup returns GraphQLNamedType | undefined, which would need a null check to read the same value.

Generated by Claude Code

eddeee888 · 2026-10-03T13:11:08Z

            contextType,
            argsType,
-          ].filter(f => f),
+          ].filter((f): f is string => !!f),


Note: I wrote the type guard out because a truthiness callback like f => f or Boolean doesn't narrow ('' is falsy, so TypeScript won't infer a predicate). Without it, the result stays (string | null | undefined)[]. The same applies to the getImports filter at L1493.

Generated by Claude Code

eddeee888 · 2026-10-03T13:11:10Z

        };
      })
-      .filter(a => a);
+      .filter(field => field !== null);


Note: Unlike the guards above, field !== null gets an inferred type predicate, so it narrows out null without one written out. It keeps the same items as a => a, because the map only ever returns null or an object.

Generated by Claude Code

eddeee888 · 2026-10-03T13:25:55Z

-      .map(gqlTypeName => ({
-        gqlType: gqlTypeName,
-        mapper: this.config.mappers[gqlTypeName],
-      }))
-      .filter(({ mapper }) => mapper.isExternal)) {
-      const externalMapper = mapper as ExternalParsedMapper;
-      const identifier = stripMapperTypeInterpolation(externalMapper.import);
-      addMapper(externalMapper.source, identifier, externalMapper.default);


Explain this runtime change.

This doesn't change runtime behaviour. It visits the same mappers in the same order and calls addMapper with the same arguments.

Before: Object.keys(mappers) was mapped to { gqlType, mapper } pairs, filtered by mapper.isExternal, and then each mapper was cast with as ExternalParsedMapper.

Now: Object.values(mappers) is looped over with if (mapper.isExternal). Object.values goes through the same own keys in the same order as Object.keys (MDN). gqlType was never read.

The cast could go because ParsedMapper is a discriminated union on isExternal: false | true (mappers.ts#L7-L13).`` Inside the if, TypeScript narrows `mapper` to `ExternalParsedMapper` (narrowing on discriminated unions). A `.filter()` callback doesn't narrow the array it returns, which is why the old code needed the cast. The only other difference is that the two intermediate arrays are no longer built.

Generated by Claude Code

eddeee888 · 2026-10-03T13:30:08Z

+          const originalArguments = original.arguments || [];
+          const argsToForceRequire = originalArguments.filter(


Why not (original.argments || []).filter like others?

original.arguments is read three times in this block: .filter here, then .length and applyOptionalFields(argsType, …) in the else if (L1702-L1714).`` All three need the fallback under strictNullChecks. I put it in one `originalArguments` local so `|| []` and its comment appear once instead of three times. The other call sites read their value only once, so they use the `|| []` inline.

I can switch this to the inline pattern too if you'd rather. It would mean (original.arguments || []) in all three places.

Generated by Claude Code

Nope, this is fine.

eddeee888 · 2026-10-03T13:30:28Z

      node: original,
      printContent: (parentNode, avoidResolverOptionals) => {
        const parentName = parentNode.name.value;
-        const parentType = this.schema.getType(parentName);


Why is this removed?

parentType was only read once, in this._federation.getMeta()[parentType.name] (L1776).`` this.schema.getType(parentName) returns `GraphQLNamedType | undefined`, so under `strictNullChecks` that read fails with TS18048. The lookup was by `parentName`, so `parentType.name` is always `parentName`. That line now indexes with `parentName` directly, and nothing else uses `parentType`, so `noUnusedLocals` would flag it if it stayed.

Generated by Claude Code

eddeee888 · 2026-10-03T13:41:58Z

    let contextType = this.getFieldContextType(parentName, node);

-    for (const directive of node.directives) {
+    // This `|| []` is only to satisfy type safety: a field without directives has none to apply


This is reading a bit weird.
node.directives || [] can definitely be undefined. So we do this fallback for type-safety, because it may happen.

If it may never happen in runtime (due to code paths, etc.), then this makes sense.

Verify whether the codepath warrants this comment or not

I checked the code path, and the comment is right. On this path node.directives is never undefined at runtime, so the || [] is only there for the type. The wording doesn't say why, though.

getContextType is only called from the FieldDefinition leave handler, at line 1683 at this commit: https://github.com/dotansimha/graphql-code-generator/blob/436ea5bdd6b76dd90de34b79ad1a3469a1b5eab2/packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts#L1683

That handler visits the AST that typescript-resolvers builds from the schema with getCachedDocumentNodeFromSchema(transformedSchema), not a parsed document: https://github.com/dotansimha/graphql-code-generator/blob/436ea5bdd6b76dd90de34b79ad1a3469a1b5eab2/packages/plugins/typescript/resolvers/src/index.ts#L125-L128

getCachedDocumentNodeFromSchema wraps @graphql-tools/utils's getDocumentNodeFromSchema. Its astFromField always sets directives: getDirectiveNodes(...), and getDirectiveNodes starts from [] and only pushes to it. So a field with no directives gets [], never undefined. See lines 135, 173 and 290 here: https://unpkg.com/@graphql-tools/utils@11.2.0/esm/print-schema-with-directives.js

oldVisit replaces the items inside directives on leave, but never the array itself.

The type still has directives?: because graphql's parse leaves it undefined when a field has no directives (graphql 17's parseDirectives). That case can't reach this visitor.

I'd reword the comment so it gives the reason instead of a tautology:

// This `|| []` is only to satisfy type safety: the schema AST from `getCachedDocumentNodeFromSchema` always sets `directives`

Reply "Ok" and I'll push that.

Generated by Claude Code

Correction to my earlier reply: that analysis only covered how typescript-resolvers calls the visitor. BaseResolversVisitor is exported, so a subclass could visit a parsed document, where directives is undefined. That makes these fallbacks real handling. ce22a28 removes the "only to satisfy type safety" comment from this one and from the other two (original.arguments and originalNode.types). The || [] fallbacks stay.

Generated by Claude Code

eddeee888 · 2026-10-03T13:58:24Z

    }
    const originalNode = parent[key] as UnionTypeDefinitionNode;
-    // This `|| []` is only to satisfy type safety: a union without members has no types to list
    const possibleTypes = (originalNode.types || [])


Drive-by: I removed the "only to satisfy type safety" comment above this fallback, because the fallback handles a real case. TsVisitor is exported:

graphql-code-generator/packages/plugins/typescript/typescript/src/visitor.ts

Line 50 in 2a97f71

export class TsVisitor<

So a caller visiting a parsed document can reach this with a union that has no types, since graphql's parse leaves them unset. This is the same fix as UnionTypeDefinition in base-resolvers-visitor.ts in this PR.

Generated by Claude Code

eddeee888 · 2026-10-03T13:58:27Z

      } else {
        this.config = {
-          // This `generates: {}` is only to satisfy type safety: a context created without `config` still fails with codegen's missing "generates" error
          generates: {},


Drive-by: I removed the "only to satisfy type safety" comment above this default, because the default is used at runtime. CodegenContext is exported:

graphql-code-generator/packages/graphql-codegen-cli/src/config.ts

Line 379 in 2a97f71

export class CodegenContext {

Its constructor takes config and graphqlConfig as optional, so a context built with neither reaches this branch with this._config undefined. generates: {} is then the actual value.

Generated by Claude Code

…sitor (#4683) BaseResolversVisitor's constructor takes `additionalConfig` as `Partial<TPluginConfig>`, dropping the `as TPluginConfig` cast and the TS2783 key-overwrite errors. `ScalarTypeDefinition`, `DirectiveDefinition` and `EnumTypeDefinition` return `string | null`, `getAbstractMembersType`'s `isTypenameNonOptional` is optional, and `defaultMapperType` throws when `defaultMapper` is not set. Nullable defaultMapper, arguments, directives, union types and mapper imports are narrowed with fallbacks or guards. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VazhaQJ1EYc8Mk39FnQxJ6 eddeee888:oss:issue-fix

…ling fallbacks `BaseResolversVisitor` is exported, so subclasses visiting other ASTs can pass nodes without `arguments`, `directives` or `types`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VazhaQJ1EYc8Mk39FnQxJ6

…efaults `TsVisitor` and `CodegenContext` are exported: a parsed union can have no `types`, and a context can be created without `config`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VazhaQJ1EYc8Mk39FnQxJ6

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 3 commits October 3, 2026 14:00

eddeee888 force-pushed the claude/hopeful-carson-wc874a branch from 2a97f71 to 9d603dc Compare October 3, 2026 14:00

eddeee888 marked this pull request as ready for review October 3, 2026 14:01

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

eddeee888 deleted the claude/hopeful-carson-wc874a branch October 3, 2026 14:12

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

Upcoming Release Changes #11008

Merged

This was referenced Oct 5, 2026

[visitor-plugin-common] fix: satisfy strict mode in base-types-visitor (#4683) #11037

Merged

[visitor-plugin-common] fix: satisfy strict mode in utils and the remaining files (#4683) #11039

Merged

		const originalArguments = original.arguments \|\| [];
		const argsToForceRequire = originalArguments.filter(

Conversation

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

Uh oh!

Description

Main

Drive-by

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!

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!

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

eddeee888 commented Oct 3, 2026 •

edited

Loading

changeset-bot Bot commented Oct 3, 2026 •

edited

Loading

github-actions Bot commented Oct 3, 2026 •

edited

Loading

🚀 Snapshot Release (`alpha`)