From 20039aeef91fdbfabce4336572f07fdb99455b93 Mon Sep 17 00:00:00 2001
From: Stephen Haberman <shaberman@homebound.com>
Date: Thu, 24 Oct 2024 14:27:23 -0500
Subject: [PATCH] fix: GraphQL setting unset children to null shouldn't make
 them dirty.

---
 src/fields/objectField.ts | 19 ++++++++++++++++---
 src/formState.test.tsx    | 14 ++++++++++++++
 2 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/src/fields/objectField.ts b/src/fields/objectField.ts
index 69e2726..173f510 100644
--- a/src/fields/objectField.ts
+++ b/src/fields/objectField.ts
@@ -3,7 +3,7 @@ import { FragmentFieldConfig, ListFieldConfig, ObjectConfig, ObjectFieldConfig,
 import { FragmentField, newFragmentField } from "src/fields/fragmentField";
 import { ListFieldState, newListFieldState } from "src/fields/listField";
 import { FieldState, FieldStateInternal, InternalSetOpts, SetOpts, newValueFieldState } from "src/fields/valueField";
-import { Builtin, deepClone, fail } from "src/utils";
+import { areEqual, Builtin, deepClone, fail } from "src/utils";
 
 /**
  * Wraps a given input/on-the-wire type `T` for editing in a form.
@@ -162,6 +162,10 @@ export function newObjectState<T, P = any>(
         maybeAutoSave,
       );
     } else if (config.type === "object") {
+      // Because our objectField will fundamentally want to do `child.firstName.set(...)` or
+      // even `child.firstName !== undefined`, etc., we "simplify" things by always setting
+      // an empty object, for our child valueFields/listFields to use/read from, although
+      // we then have to "oh right ignore {}" in places like `dirty`.
       if (!instance[key]) {
         instance[key] = {} as any;
       }
@@ -263,7 +267,12 @@ export function newObjectState<T, P = any>(
     },
 
     get dirty(): boolean {
-      return getFields(this).some((f) => f.dirty) || this.isUnset();
+      return (
+        getFields(this).some((f) => f.dirty) ||
+        // `isUnset` checks if our `parent[key] === undefined`, which can mean "surely we're dirty",
+        // but as long as we've got some keys actually set
+        (this.isUnset() && !areEqual(this.value, {}))
+      );
     },
 
     get isNewEntity(): boolean {
@@ -342,7 +351,11 @@ export function newObjectState<T, P = any>(
           // look dirty, but if we're new we should include them anyway.
           (this.isNewEntity && (f as any)._kind === "value" && f.value !== undefined) ||
           // ...or they're non-empty sub-objects
-          (this.isNewEntity && (f as any)._kind === "object" && Object.entries(f.changedValue).length > 0) ||
+          (this.isNewEntity &&
+            (f as any)._kind === "object" &&
+            // Child objects that are unset will have `changedValue = null`
+            f.changedValue &&
+            Object.entries(f.changedValue).length > 0) ||
           // ...or they're non-empty sub-lists
           (this.isNewEntity && (f as any)._kind === "list" && f.value?.length > 0)
         ) {
diff --git a/src/formState.test.tsx b/src/formState.test.tsx
index 56c8c02..c64523d 100644
--- a/src/formState.test.tsx
+++ b/src/formState.test.tsx
@@ -679,6 +679,20 @@ describe("formState", () => {
     expect(a1.dirty).toBeFalsy();
   });
 
+  it("realizes object fields set to null aren't actually dirty", () => {
+    // Given we started off with a `object: undefined`
+    const a1 = createAuthorWithAddressInputState({ address: undefined });
+    expect(a1.address.dirty).toBe(false);
+    expect(a1.address.value).toEqual({});
+    // And it's later set to null, i.e. from a mutation returning `{ address: null }`
+    a1.set({ firstName: "a1", address: null });
+    // Then we convert it to `{}` to potentially hold data
+    expect(a1.address.value).toEqual({});
+    // But we don't consider the address dirty
+    expect(a1.address.dirty).toBe(false);
+    expect(a1.changedValue).toEqual({ firstName: "a1" });
+  });
+
   it("knows an object's field of type object is dirty", () => {
     const a1 = createAuthorInputState({
       books: [{ title: "b1", classification: dd100 }],