Skip to content

Stabilize queries generated object typedefs #1298

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 27, 2025

Conversation

scotttrinh
Copy link
Collaborator

@scotttrinh scotttrinh commented Jun 26, 2025

Different instances can produce different field orders for Object codecs, so we should have an fully ordered sorting algorithm to stabilize the order. This should help codegen to be deterministic, or at least get us a little closer.

The sorting is now grouped in a logic manner like this:

  • Required single properties
  • Optional single properties
  • Required multi properties
  • Optional multi properties
  • Required single links
  • Optional single links
  • Required multi links
  • Optional multi links

Within those groups, fields are sorted by name.


After:

export type GetMoviesStarringReturns = Array<{
  "id": string;
  "local_date": LocalDate;
  "range": Range<number>;
  "release_year": number;
  "title": string;
  "tuple": [number, string, Array<bigint>];
  "version": {
    "major": number;
    "minor": number;
    "stage": ("dev" | "alpha" | "beta" | "rc" | "final");
    "stage_no": number;
    "local": Array<string>;
  };
  "characters": Array<{
    "name": string;
    "@character_name": string | null;
    "height": string | null;
  }>;
}>;

Before:

export type GetMoviesStarringReturns = Array<{
  "id": string;
  "title": string;
  "release_year": number;
  "characters": Array<{
    "name": string;
    "height": string | null;
    "@character_name": string | null;
  }>;
  "tuple": [number, string, Array<bigint>];
  "version": {
    "major": number;
    "minor": number;
    "stage": ("dev" | "alpha" | "beta" | "rc" | "final");
    "stage_no": number;
    "local": Array<string>;
  };
  "range": Range<number>;
  "local_date": LocalDate;
}>;

Diff:

diff -u /var/folders/48/hsh1f1017yb145xnhq92rpkc0000gn/T/buffer-content-OCcfJT /var/folders/48/hsh1f1017yb145xnhq92rpkc0000gn/T/buffer-content-YPeMUL
--- /var/folders/48/hsh1f1017yb145xnhq92rpkc0000gn/T/buffer-content-OCcfJT	2025-06-26 16:21:57.498563798 -0400
+++ /var/folders/48/hsh1f1017yb145xnhq92rpkc0000gn/T/buffer-content-YPeMUL	2025-06-26 16:21:57.499198554 -0400
@@ -1,12 +1,9 @@
 export type GetMoviesStarringReturns = Array<{
   "id": string;
-  "title": string;
+  "local_date": LocalDate;
+  "range": Range<number>;
   "release_year": number;
-  "characters": Array<{
-    "name": string;
-    "height": string | null;
-    "@character_name": string | null;
-  }>;
+  "title": string;
   "tuple": [number, string, Array<bigint>];
   "version": {
     "major": number;
@@ -15,6 +12,9 @@
     "stage_no": number;
     "local": Array<string>;
   };
-  "range": Range<number>;
-  "local_date": LocalDate;
+  "characters": Array<{
+    "name": string;
+    "@character_name": string | null;
+    "height": string | null;
+  }>;
 }>;
\ No newline at end of file

Diff finished.  Thu Jun 26 16:21:57 2025

@scotttrinh scotttrinh requested review from 1st1 and Copilot June 26, 2025 19:11
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request introduces a deterministic sorting mechanism for generating object typedefs by grouping and ordering codec fields consistently. Key changes include the definition of a new FieldDef type, the introduction of a sorting function (getSortPriority) for codec fields, and updates to object generation functions to use the sorted fields.

Comments suppressed due to low confidence (1)

packages/gel/src/reflection/analyzeQuery.ts:263

  • [nitpick] Adding inline comments in the unwrapSetCodec function to clarify why only fields with cardinality 'Many' or 'AtLeastOne' are unwrapped could help maintainers understand the decision logic better.
function unwrapSetCodec(field: FieldDef) {

Different instances can produce different field orders for Object
codecs, so we should have an fully ordered sorting algorithm to
stabilize the order. This should help codegen to be deterministic, or at
least get us a little closer.
@scotttrinh scotttrinh force-pushed the generate-stable-queries-field-order branch from 7e43ef1 to e6658fe Compare June 26, 2025 19:12
Copy link
Member

@1st1 1st1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it does make sense to add a test -- like we must make sure we never reorder named tuples... :)

@1st1
Copy link
Member

1st1 commented Jun 26, 2025

like we must make sure we never reorder named tuples... :)

Although... I guess JS doesn't care about positional access for named tuples.

@scotttrinh
Copy link
Collaborator Author

Maybe it does make sense to add a test -- like we must make sure we never reorder named tuples... :)

Although... I guess JS doesn't care about positional access for named tuples.

Yeah, exactly. Since types are structural, we can't easily detect if the order of properties is different other than comparing strings. We do already test it functionally, so hopefully that will cover this specific regression idea.

@scotttrinh scotttrinh merged commit 0076d38 into master Jun 27, 2025
9 checks passed
@scotttrinh scotttrinh deleted the generate-stable-queries-field-order branch June 27, 2025 13:54
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.

2 participants