fix: update visitor logic in topologicalSortAST to fix Issue #528

Currently, there is a problem described in Issue #528 whereby "const"
output can have some variables used before they're defined. There were
two related issues in the `visit` invocation in `topologicalSortAST`
that caused the problem. These changes resolve "use before defined"
errors in generated output and enhance compatibility with custom scalars
and circular type definitions.

- Recursively unwrap GraphQL types (List/NonNull) via `getNamedType` to
correctly register dependencies.
- Add `INTERFACE_TYPE_DEFINITION` to the list of target kinds for proper
dependency handling.

Additionally, this commit updates the `topsort` algo to iterate over all
nodes (`g.nodes().forEach(visit)`) instead of just sinks, ensuring
self-referential and cyclic dependencies (e.g. self-circular inputs) are
processed.

Since these changes affect output, impacted tests have had their
expect-values updated as well (just re-ordering of certain type
declarations).

Author: trevor-anderson

src/graphql.ts

@@ -14,12 +14,23 @@ import type {
 import { Graph } from 'graphlib';
 import {
   isSpecifiedScalarType,
+  Kind,
   visit,
 } from 'graphql';
 
-export const isListType = (typ?: TypeNode): typ is ListTypeNode => typ?.kind === 'ListType';
-export const isNonNullType = (typ?: TypeNode): typ is NonNullTypeNode => typ?.kind === 'NonNullType';
-export const isNamedType = (typ?: TypeNode): typ is NamedTypeNode => typ?.kind === 'NamedType';
+/**
+ * Recursively unwraps a GraphQL type until it reaches the NamedType.
+ *
+ * Since a GraphQL type is defined as either a NamedTypeNode, ListTypeNode, or NonNullTypeNode,
+ * this implementation safely recurses until the underlying NamedTypeNode is reached.
+ */
+function getNamedType(typeNode: TypeNode): NamedTypeNode {
+  return typeNode.kind === Kind.NAMED_TYPE ? typeNode : getNamedType(typeNode.type);
+}
+
+export const isListType = (typ?: TypeNode): typ is ListTypeNode => typ?.kind === Kind.LIST_TYPE;
+export const isNonNullType = (typ?: TypeNode): typ is NonNullTypeNode => typ?.kind === Kind.NON_NULL_TYPE;
+export const isNamedType = (typ?: TypeNode): typ is NamedTypeNode => typ?.kind === Kind.NAMED_TYPE;
 
 export const isInput = (kind: string) => kind.includes('Input');
 
@@ -48,44 +59,50 @@ export function InterfaceTypeDefinitionBuilder(useInterfaceTypes: boolean | unde
 export function topologicalSortAST(schema: GraphQLSchema, ast: DocumentNode): DocumentNode {
   const dependencyGraph = new Graph();
   const targetKinds = [
-    'ObjectTypeDefinition',
-    'InputObjectTypeDefinition',
-    'EnumTypeDefinition',
-    'UnionTypeDefinition',
-    'ScalarTypeDefinition',
+    Kind.OBJECT_TYPE_DEFINITION,
+    Kind.INPUT_OBJECT_TYPE_DEFINITION,
+    Kind.INTERFACE_TYPE_DEFINITION,
+    Kind.SCALAR_TYPE_DEFINITION,
+    Kind.ENUM_TYPE_DEFINITION,
+    Kind.UNION_TYPE_DEFINITION,
   ];
 
   visit<DocumentNode>(ast, {
     enter: (node) => {
       switch (node.kind) {
-        case 'ObjectTypeDefinition':
-        case 'InputObjectTypeDefinition': {
+        case Kind.OBJECT_TYPE_DEFINITION:
+        case Kind.INPUT_OBJECT_TYPE_DEFINITION:
+        case Kind.INTERFACE_TYPE_DEFINITION: {
           const typeName = node.name.value;
           dependencyGraph.setNode(typeName);
 
           if (node.fields) {
             node.fields.forEach((field) => {
-              if (field.type.kind === 'NamedType') {
-                const dependency = field.type.name.value;
-                const typ = schema.getType(dependency);
-                if (typ?.astNode?.kind === undefined || !targetKinds.includes(typ.astNode.kind))
-                  return;
-
-                if (!dependencyGraph.hasNode(dependency))
-                  dependencyGraph.setNode(dependency);
+              // Unwrap the type
+              const namedTypeNode = getNamedType(field.type);
+              const dependency = namedTypeNode.name.value;
+              const namedType = schema.getType(dependency);
+              if (
+                namedType?.astNode?.kind === undefined
+                || !targetKinds.includes(namedType.astNode.kind)
+              ) {
+                return;
+              }
 
-                dependencyGraph.setEdge(typeName, dependency);
+              if (!dependencyGraph.hasNode(dependency)) {
+                dependencyGraph.setNode(dependency);
               }
+              dependencyGraph.setEdge(typeName, dependency);
             });
           }
           break;
         }
-        case 'ScalarTypeDefinition':
-        case 'EnumTypeDefinition': {
+        case Kind.SCALAR_TYPE_DEFINITION:
+        case Kind.ENUM_TYPE_DEFINITION: {
           dependencyGraph.setNode(node.name.value);
           break;
         }
-        case 'UnionTypeDefinition': {
+        case Kind.UNION_TYPE_DEFINITION: {
           const dependency = node.name.value;
           if (!dependencyGraph.hasNode(dependency))
             dependencyGraph.setNode(dependency);
@@ -111,7 +128,7 @@ export function topologicalSortAST(schema: GraphQLSchema, ast: DocumentNode): Do
   // Create a map of definitions for quick access, using the definition's name as the key.
   const definitionsMap: Map<string, DefinitionNode> = new Map();
 
-  // SCHEMA_DEFINITION does not have name.
+  // SCHEMA_DEFINITION does not have a name.
   // https://spec.graphql.org/October2021/#sec-Schema
   const astDefinitions = ast.definitions.filter(def => def.kind !== 'SchemaDefinition');
 
@@ -120,7 +137,7 @@ export function topologicalSortAST(schema: GraphQLSchema, ast: DocumentNode): Do
       definitionsMap.set(definition.name.value, definition);
   });
 
-  // Two arrays to store sorted and not sorted definitions.
+  // Two arrays to store sorted and unsorted definitions.
   const sortedDefinitions: DefinitionNode[] = [];
   const notSortedDefinitions: DefinitionNode[] = [];
 
@@ -141,7 +158,7 @@ export function topologicalSortAST(schema: GraphQLSchema, ast: DocumentNode): Do
 
   if (newDefinitions.length !== astDefinitions.length) {
     throw new Error(
-      `unexpected definition length after sorted: want ${astDefinitions.length} but got ${newDefinitions.length}`,
+      `Unexpected definition length after sorting: want ${astDefinitions.length} but got ${newDefinitions.length}`,
     );
   }
 
@@ -155,33 +172,35 @@ function hasNameField(node: ASTNode): node is DefinitionNode & { name?: NameNode
   return 'name' in node;
 }
 
-// Re-implemented w/o CycleException version
-// https://github.com/dagrejs/graphlib/blob/8d27cb89029081c72eb89dde652602805bdd0a34/lib/alg/topsort.js
+/**
+ * [Re-implemented topsort function][topsort-ref] with cycle handling. This version iterates over
+ * all nodes in the graph to ensure every node is visited, even if the graph contains cycles.
+ *
+ * [topsort-ref]: https://github.com/dagrejs/graphlib/blob/8d27cb89029081c72eb89dde652602805bdd0a34/lib/alg/topsort.js
+ */
 export function topsort(g: Graph): string[] {
-  const visited: Record<string, boolean> = {};
-  const stack: Record<string, boolean> = {};
-  const results: any[] = [];
+  const visited = new Set<string>();
+  const results: Array<string> = [];
 
   function visit(node: string) {
-    if (!(node in visited)) {
-      stack[node] = true;
-      visited[node] = true;
-      const predecessors = g.predecessors(node);
-      if (Array.isArray(predecessors))
-        predecessors.forEach(node => visit(node));
-
-      delete stack[node];
-      results.push(node);
-    }
+    if (visited.has(node))
+      return;
+    visited.add(node);
+    // Recursively visit all predecessors of the node.
+    g.predecessors(node)?.forEach(visit);
+    results.push(node);
   }
 
-  g.sinks().forEach(node => visit(node));
+  // Visit every node in the graph (instead of only sinks)
+  g.nodes().forEach(visit);
 
   return results.reverse();
 }
 
 export function isGeneratedByIntrospection(schema: GraphQLSchema): boolean {
-  return Object.entries(schema.getTypeMap()).filter(([name, type]) => !name.startsWith('__') && !isSpecifiedScalarType(type)).every(([, type]) => type.astNode === undefined)
+  return Object.entries(schema.getTypeMap())
+    .filter(([name, type]) => !name.startsWith('__') && !isSpecifiedScalarType(type))
+    .every(([, type]) => type.astNode === undefined)
 }
 
 // https://spec.graphql.org/October2021/#EscapedCharacter

tests/graphql.spec.ts

@@ -235,13 +235,13 @@ describe('topologicalSortAST', () => {
     const sortedSchema = getSortedSchema(schema);
 
     const expectedSortedSchema = dedent/* GraphQL */`
-      input A {
-        a: A
-      }
-
       input B {
         b: B
       }
+
+      input A {
+        a: A
+      }
     `;
 
     expect(sortedSchema).toBe(expectedSortedSchema);

tests/myzod.spec.ts

@@ -1506,12 +1506,6 @@ describe('myzod', () => {
     );
     expect(removedInitialEmitValue(result.content)).toMatchInlineSnapshot(`
       "
-      export const UserCreateInputSchema: myzod.Type<UserCreateInput> = myzod.object({
-          name: myzod.string(),
-          date: myzod.date(),
-          email: myzod.string().email()
-      });
-
       export const UserSchema: myzod.Type<User> = myzod.object({
           __typename: myzod.literal('User').optional(),
           id: myzod.string(),
@@ -1521,6 +1515,12 @@ describe('myzod', () => {
           isMember: myzod.boolean().optional().nullable(),
           createdAt: myzod.date()
       });
+
+      export const UserCreateInputSchema: myzod.Type<UserCreateInput> = myzod.object({
+          name: myzod.string(),
+          date: myzod.date(),
+          email: myzod.string().email()
+      });
       "
     `)
     for (const wantNotContain of ['Query', 'Mutation', 'Subscription'])

tests/yup.spec.ts

@@ -1486,6 +1486,7 @@ describe('yup', () => {
       },
       {},
     );
+
     expect(result.content).toMatchInlineSnapshot(`
       "
 
@@ -1495,12 +1496,6 @@ describe('yup', () => {
         }).defined()
       }
 
-      export const UserCreateInputSchema: yup.ObjectSchema<UserCreateInput> = yup.object({
-          name: yup.string().defined().nonNullable(),
-          date: yup.date().defined().nonNullable(),
-          email: yup.string().email().defined().nonNullable()
-      });
-
       export const UserSchema: yup.ObjectSchema<User> = yup.object({
           __typename: yup.string<'User'>().optional(),
           id: yup.string().defined().nonNullable(),
@@ -1510,6 +1505,12 @@ describe('yup', () => {
           isMember: yup.boolean().defined().nullable().optional(),
           createdAt: yup.date().defined().nonNullable()
       });
+
+      export const UserCreateInputSchema: yup.ObjectSchema<UserCreateInput> = yup.object({
+          name: yup.string().defined().nonNullable(),
+          date: yup.date().defined().nonNullable(),
+          email: yup.string().email().defined().nonNullable()
+      });
       "
     `)
 

tests/zod.spec.ts

@@ -1732,14 +1732,9 @@ describe('zod', () => {
       },
       {},
     );
+
     expect(removedInitialEmitValue(result.content)).toMatchInlineSnapshot(`
       "
-      export const UserCreateInputSchema: z.ZodObject<Properties<UserCreateInput>> = z.object({
-          name: z.string(),
-          date: z.date(),
-          email: z.string().email()
-      });
-
       export const UserSchema: z.ZodObject<Properties<User>> = z.object({
           __typename: z.literal('User').optional(),
           id: z.string(),
@@ -1749,6 +1744,12 @@ describe('zod', () => {
           isMember: z.boolean().nullish(),
           createdAt: z.date()
       });
+
+      export const UserCreateInputSchema: z.ZodObject<Properties<UserCreateInput>> = z.object({
+          name: z.string(),
+          date: z.date(),
+          email: z.string().email()
+      });
       "
     `)