Skip to content

fix(visitor-plugin-common): handles more cases in conditional spread of a fragment#10839

Open
vkbansal-rubrik wants to merge 1 commit into
dotansimha:masterfrom
vkbansal-rubrik:fix-fragment-issue
Open

fix(visitor-plugin-common): handles more cases in conditional spread of a fragment#10839
vkbansal-rubrik wants to merge 1 commit into
dotansimha:masterfrom
vkbansal-rubrik:fix-fragment-issue

Conversation

@vkbansal-rubrik
Copy link
Copy Markdown

Description

Fixes TypeError: Unexpected type. thrown from
@graphql-codegen/visitor-plugin-common's selection-set-to-object.ts
buildSelectionSet when a fragment is spread with a conditional
directive (@include / @skip / @defer) and the spread fragment's
top-level selection set contains an InlineFragment (... on X { ... })
or another FragmentSpread (...Other).

Root cause

buildFragmentSpreadsUsage only wraps top-level FIELD nodes from the
spread fragment with fragmentDirectives; INLINE_FRAGMENT and
FRAGMENT_SPREAD nodes pass through as raw AST. For a conditional
spread, _buildGroupedSelections then inlines those raw nodes into
flattenedSelectionNodes and feeds the result to buildSelectionSet,
which only handles FIELD / DIRECTIVE for nodes with a kind
anything else hits throw new TypeError('Unexpected type.').

The non-conditional spread path already does the right thing: it routes
FragmentSpreadUsage.selectionNodes through flattenSelectionSet
recursively (which expands inline fragments and nested spreads per
type) before consuming them.

Fix

In _buildGroupedSelections, after a conditional FragmentSpreadUsage
is inlined into flattenedSelectionNodes, partition the result and
route top-level INLINE_FRAGMENT / FRAGMENT_SPREAD nodes through the
existing flattenSelectionSet pipeline before handing the FIELD-only
merged set to buildSelectionSet. buildSelectionSet's contract — that
raw AST nodes are FIELD or DIRECTIVE — is preserved, and FIELD-only
callers are byte-for-byte unaffected.

Related # (issue) (replace with the issue number after filing the
bug report)


Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Screenshots/Sandbox (if appropriate/relevant):

Minimal reproduction repo (follows the layout of
graphql-code-generator-issue-sandbox-template):

https://github.com/vkbansal-rubrik/codegen-conditional-spread-bug

$ git clone https://github.com/vkbansal-rubrik/codegen-conditional-spread-bug.git
$ cd codegen-conditional-spread-bug
$ pnpm install
$ pnpm generate

❯ Generate
✖ Generate [FAILED: Unexpected type.]

With this PR applied, pnpm generate completes and emits a valid
LibraryQuery type whose featured field is a union of the
type-narrowed members plus an empty-object variant covering
$includeFeatured = false.


How Has This Been Tested?

Two new test cases added to
packages/plugins/typescript/operations/tests/ts-documents.skip-include-directives.spec.ts,
both inside the existing TypeScript Operations Plugin - @include directives
describe block. Each uses a generic Book | Magazine (Publication) schema
with a Library parent — mirroring the regression in different shapes:

  • Test A — top-level inline fragments:
    handles conditional spread of a fragment whose top-level selections contain inline fragments.
    The spread fragment contains ... on Book { ... } and
    ... on Magazine { ... } at its top level and is conditionally spread
    via @include(if: $includeFeatured). Without the fix this throws
    Unexpected type.; with the fix the generated TypeScript compiles
    and exposes the union members.
  • Test B — top-level fragment spreads:
    handles conditional spread of a fragment whose top-level selections are fragment spreads.
    Same shape as Test A, but the top-level inline fragments are
    replaced with named fragment spreads (...BookFragment /
    ...MagazineFragment). Demonstrates that converting inline
    fragments to named ones is not a sufficient workaround under
    v6 — the same throw fires — and verifies the fix covers both
    shapes.

Manual verification matrix

Before the fix:

× handles conditional spread of a fragment whose top-level selections contain inline fragments
× handles conditional spread of a fragment whose top-level selections are fragment spreads
  TypeError: Unexpected type.
      at SelectionSetToObject.buildSelectionSet selection-set-to-object.ts:805

After the fix:

Suite Result
New tests (the two added in this PR) 2/2 pass
ts-documents.skip-include-directives.spec.ts (whole file) 20/20 pass
@graphql-codegen/typescript-operations (full suite) 236/237 pass — the 1 remaining failure (imports external custom scalar in shared type file when said scalar is used in relevant Input) also fails on master without this patch, so it is unrelated to this change.
@graphql-codegen/visitor-plugin-common (full suite) 50/50 pass

Test Environment:

  • OS: macOS (Darwin 25.5.0)
  • @graphql-codegen/...:
    • @graphql-codegen/visitor-plugin-common (patched here)
    • @graphql-codegen/typescript-operations@6.0.2 (new tests here)
    • @graphql-codegen/cli@7.0.0
  • NodeJS: 24.13.0 (also reproduced on 20.x and 22.x)
  • graphql: 16.14.0 (also reproduces on 15.8.0)

Checklist:

  • I have followed the
    CONTRIBUTING doc and the
    style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas — the new
    partition/re-flatten block in _buildGroupedSelections carries an inline comment
    explaining why it's needed (the buildSelectionSet contract).
  • I have made corresponding changes to the documentation — N/A (no public API change).
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules — N/A
    (single-package change inside visitor-plugin-common; consumers pick it up via the
    bumped version).

Further comments

Why this approach over alternatives

Two alternatives I considered and rejected:

  1. Expand non-FIELD top-level selections inside buildFragmentSpreadsUsage
    recursively flatten any top-level INLINE_FRAGMENT / FRAGMENT_SPREAD there, so
    FragmentSpreadUsage.selectionNodes always contains FIELD-shaped entries.
    Cleaner contract for that type, but the change is significantly larger and touches
    the non-conditional path (which is already correct via the recursive
    flattenSelectionSet call in buildSelectionSet's fragment-spread-usage branch).
    I chose to keep the fix surgical to the broken branch.

  2. Teach buildSelectionSet to handle INLINE_FRAGMENT / FRAGMENT_SPREAD kinds
    directly at the throw site
    — would mean duplicating the per-type narrowing logic
    that flattenSelectionSet + _collectInlineFragments already implement. Routing
    through the existing pipeline reuses that logic instead.

Edge case for maintainer review

@include / @skip on an inline fragment inside a conditionally-spread fragment
(e.g. ... on X @include(if: $x) { ... } at the top level of a fragment spread with
its own @include) currently goes through flattenSelectionSet's
inlineFragmentConditionalSelections path, so those fields end up in
selectionNodesByTypeNameConditional, not in the nestedByType map this fix reads.
That nested-conditional case isn't exercised by the new tests or the linked repro and
isn't fixed here. Happy to extend the fix if you consider it in scope.

Workaround documented in the repro

Until this lands, users can restructure their fragments so the top-level selection
set contains only FIELD nodes — i.e., split type-narrowing inline fragments into
per-concrete-type helper fragments and spread each helper directly at the consumer.
The repro README walks through this.

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 18, 2026

🦋 Changeset detected

Latest commit: 7338100

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/visitor-plugin-common Patch
@graphql-codegen/typescript-operations 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

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.

1 participant