fix(visitor-plugin-common): handles more cases in conditional spread of a fragment#10839
Open
vkbansal-rubrik wants to merge 1 commit into
Open
fix(visitor-plugin-common): handles more cases in conditional spread of a fragment#10839vkbansal-rubrik wants to merge 1 commit into
vkbansal-rubrik wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: 7338100 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes
TypeError: Unexpected type.thrown from@graphql-codegen/visitor-plugin-common'sselection-set-to-object.tsbuildSelectionSetwhen a fragment is spread with a conditionaldirective (
@include/@skip/@defer) and the spread fragment'stop-level selection set contains an
InlineFragment(... on X { ... })or another
FragmentSpread(...Other).Root cause
buildFragmentSpreadsUsageonly wraps top-levelFIELDnodes from thespread fragment with
fragmentDirectives;INLINE_FRAGMENTandFRAGMENT_SPREADnodes pass through as raw AST. For a conditionalspread,
_buildGroupedSelectionsthen inlines those raw nodes intoflattenedSelectionNodesand feeds the result tobuildSelectionSet,which only handles
FIELD/DIRECTIVEfor nodes with akind—anything else hits
throw new TypeError('Unexpected type.').The non-conditional spread path already does the right thing: it routes
FragmentSpreadUsage.selectionNodesthroughflattenSelectionSetrecursively (which expands inline fragments and nested spreads per
type) before consuming them.
Fix
In
_buildGroupedSelections, after a conditionalFragmentSpreadUsageis inlined into
flattenedSelectionNodes, partition the result androute top-level
INLINE_FRAGMENT/FRAGMENT_SPREADnodes through theexisting
flattenSelectionSetpipeline before handing the FIELD-onlymerged set to
buildSelectionSet.buildSelectionSet's contract — thatraw AST nodes are
FIELDorDIRECTIVE— is preserved, and FIELD-onlycallers are byte-for-byte unaffected.
Related # (issue) (replace with the issue number after filing the
bug report)
Type of change
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
With this PR applied,
pnpm generatecompletes and emits a validLibraryQuerytype whosefeaturedfield is a union of thetype-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 directivesdescribe block. Each uses a generic
Book | Magazine(Publication) schemawith a
Libraryparent — mirroring the regression in different shapes: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 spreadvia
@include(if: $includeFeatured). Without the fix this throwsUnexpected type.; with the fix the generated TypeScript compilesand exposes the union members.
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 inlinefragments 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:
After the fix:
ts-documents.skip-include-directives.spec.ts(whole file)@graphql-codegen/typescript-operations(full suite)imports external custom scalar in shared type file when said scalar is used in relevant Input) also fails onmasterwithout this patch, so it is unrelated to this change.@graphql-codegen/visitor-plugin-common(full suite)Test Environment:
@graphql-codegen/...:@graphql-codegen/visitor-plugin-common(patched here)@graphql-codegen/typescript-operations@6.0.2(new tests here)@graphql-codegen/cli@7.0.0graphql: 16.14.0 (also reproduces on 15.8.0)Checklist:
CONTRIBUTING doc and the
style guidelines of this project
partition/re-flatten block in
_buildGroupedSelectionscarries an inline commentexplaining why it's needed (the
buildSelectionSetcontract).(single-package change inside
visitor-plugin-common; consumers pick it up via thebumped version).
Further comments
Why this approach over alternatives
Two alternatives I considered and rejected:
Expand non-
FIELDtop-level selections insidebuildFragmentSpreadsUsage—recursively flatten any top-level
INLINE_FRAGMENT/FRAGMENT_SPREADthere, soFragmentSpreadUsage.selectionNodesalways containsFIELD-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
flattenSelectionSetcall inbuildSelectionSet's fragment-spread-usage branch).I chose to keep the fix surgical to the broken branch.
Teach
buildSelectionSetto handleINLINE_FRAGMENT/FRAGMENT_SPREADkindsdirectly at the throw site — would mean duplicating the per-type narrowing logic
that
flattenSelectionSet+_collectInlineFragmentsalready implement. Routingthrough the existing pipeline reuses that logic instead.
Edge case for maintainer review
@include/@skipon an inline fragment inside a conditionally-spread fragment(e.g.
... on X @include(if: $x) { ... }at the top level of a fragment spread withits own
@include) currently goes throughflattenSelectionSet'sinlineFragmentConditionalSelectionspath, so those fields end up inselectionNodesByTypeNameConditional, not in thenestedByTypemap 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
FIELDnodes — i.e., split type-narrowing inline fragments intoper-concrete-type helper fragments and spread each helper directly at the consumer.
The repro README walks through this.