fix(graphql): keep distinct GraphQL types with identical shape separate#3044
Open
schani wants to merge 3 commits into
Open
fix(graphql): keep distinct GraphQL types with identical shape separate#3044schani wants to merge 3 commits into
schani wants to merge 3 commits into
Conversation
…te (#1072) Distinct GraphQL object types (e.g. GridConfig and GridProps) that happen to share the same fields were unified into a single output class, since quicktype's class deduplication only compared structural shape. This is correct for JSON/JSON Schema input, but GraphQL input should always keep schema-distinct types distinct since they are known-unrelated by the schema itself, regardless of --combine-classes (which only governs combining structurally-similar-but-not-identical JSON-derived classes). Tag each class built from GraphQL input with an identity type attribute carrying its GraphQL schema type name, and make both class type identity (TypeBuilder.getClassType) and the CombineClasses rewrite respect it, so two GraphQL classes are only unified when they come from the same named GraphQL type. Repeated uses of the same GraphQL type still dedupe as before. Added a GraphQL fixture (query + introspection schema) with two structurally-identical sibling types, and a focused unit test asserting the generated TypeScript keeps them as separate interfaces with both combineClasses on and off. Co-Authored-By: gpt-5.6-sol via pi <noreply@openai.com>
Generated-output differences111 files differ — 43 modified, 62 new, 6 deleted |
Generated-output differences111 files differ — 43 modified, 62 new, 6 deleted |
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.
Bug
Distinct GraphQL object types with identical structure were collapsed into a single output class. For example:
produced a single merged
GridConfigClassused for bothconfigandprops, instead of two separateGridConfig/GridPropsinterfaces.--no-combine-classeshad no effect, since that flag only applies to JSON input.Root cause
quicktype's class type identity/deduplication (
classTypeIdentityinTypeBuilder.getClassType) and theCombineClassesgraph-rewrite pass both compared classes purely by structural shape (their properties). For JSON/JSON Schema input this is desired — structurally-identical inferred classes should unify. But GraphQL input already knows which types are distinct (from the schema itself), and separate GraphQL types should never be unified just because they happen to have the same fields.Fix
packages/quicktype-graphql-input/src/index.ts: each class built from a GraphQL object type is now tagged with a new identityTypeAttributeKind(graphqlTypeName) carrying the GraphQL schema type's name.packages/quicktype-core/src/Type/Type.ts: addedhaveSameIdentityAttributes(a, b)helper. Class type identity inTypeBuilder.getClassTypealready includes identity attributes, so classes with different GraphQL type names are never unified there.packages/quicktype-core/src/rewrites/CombineClasses.ts: theCombineClassesrewrite pass now also requireshaveSameIdentityAttributesbefore considering two class prototypes combinable, so structurally-identical-but-distinct GraphQL classes are not merged there either. Repeated uses of the same GraphQL type still dedupe as before, since they carry the same identity attribute.This only affects GraphQL input; JSON/JSON Schema-derived classes carry no such identity attribute, so their existing structural deduplication/combining behavior is unchanged.
Test coverage
test/inputs/graphql/separate-types1.graphql+separate-types.gqlschema+separate-types1.1.json, modeled on the issue'sGrid/GridConfig/GridPropsexample, wired into the existinggraphqlfixture mechanism (test/fixtures.ts).test/unit/graphql-separate-types.test.ts: renders TypeScript from that GraphQL input withcombineClassesbothtrueandfalse, and asserts the output keepsConfigandPropsas two separate interfaces, while a genuinely-repeated type (Item) still dedupes to one interface.Verification
npm run build— passes.npm run test:unit— 172/172 tests pass.QUICKTEST=true FIXTURE=graphql-typescript script/test— 10/10 tests pass (including the newseparate-types1.graphqlfixture).GridConfig/GridPropsare now emitted as separate interfaces (both with and without--no-combine-classes), while the sharedComponentReftype used by both remains a single interface.Fixes #1072
🤖 Generated with Claude Code