fix: update visitor logic in topologicalSortAST to fix Issue #528
#958
Conversation
…ex#528 Currently, there is a problem described in Issue Code-Hex#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).
| @@ -235,13 +235,13 @@ describe('topologicalSortAST', () => { | |||
| const sortedSchema = getSortedSchema(schema); | |||
|
|
|||
| const expectedSortedSchema = dedent/* GraphQL */` | |||
| input A { | |||
There was a problem hiding this comment.
Why do I need to change this test case? I expect this test case to pass.
There was a problem hiding this comment.
The relevant code change is in the topsort algo fn, where g.sinks().forEach(visit) is replaced with g.nodes().forEach(visit) — that change makes this necessary.
The reason for the change is that, in order to ensure that every node is visited, we need to iterate over all nodes in the graph. In the presence of cycles, some nodes may not be considered sinks (i.e., they have no outgoing edges), and therefore would be skipped in a traditional topological sort. This is precisely what was happening in this test case with the old implementation — it's why input A and input B were not re-ordered like all the other test cases.
The new g.nodes() approach guarantees that every definition is included. The difference is in the starting point for the DFS that produces the order:
-
Using
g.sinks().forEach(visit):
This approach starts the DFS only from nodes that are sinks — that is, nodes with no outgoing edges that do not depend on any other nodes. In a graph without cycles, sinks are safe starting points because they can be placed first in the order. However, if a node is part of a cycle — or self-references, as in the relevant test-case — it will have an outgoing edge (even if that edge is to itself), so it will not be considered a sink. That means the DFS might never start from those nodes, and they could be entirely skipped in the ordering. This is whyinput Aandinput Bwere not re-ordered in the previous implementation. -
Using
g.nodes().forEach(visit):
This approach goes over every node in the graph and ensures that each one is visited, regardless of whether it’s a sink or not. This is especially important for self-circular or mutually dependent nodes, because even if they are in a cycle (and therefore not sinks), they’ll still be processed and appear in the final list.
There was a problem hiding this comment.
Thanks for your explanation!
Let's Go!!
| case 'InputObjectTypeDefinition': { | ||
| case Kind.OBJECT_TYPE_DEFINITION: | ||
| case Kind.INPUT_OBJECT_TYPE_DEFINITION: | ||
| case Kind.INTERFACE_TYPE_DEFINITION: { |
There was a problem hiding this comment.
This is a new case, so please add a test to confirm that the interface is also sorted
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
visitinvocation intopologicalSortASTthat caused the problem. These changes resolve "use before defined" errors in generated output and enhance compatibility with custom scalars and circular type definitions.getNamedTypeto correctly register dependencies.INTERFACE_TYPE_DEFINITIONto the list of target kinds for proper dependency handling.Additionally, this commit updates the
topsortalgo 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).