fix(form): fix issue making circular structures sometimes causing inf…

…inite loop (#6699)

* test(form): improve test for immutableReconcile handling cyclic structures

Co-authored-by: @stian-svedenborg <deinoff@gmail.com>

* fix(sanity): add fix proposed in #6646

Co-authored-by: @sarahsvedenborg <sarah.svedenborg@gmail.com>

---------

Co-authored-by: @stian-svedenborg <deinoff@gmail.com>
Co-authored-by: @sarahsvedenborg <sarah.svedenborg@gmail.com>

packages/sanity/src/core/form/store/utils/__tests__/immutableReconcile.test.ts

@@ -29,21 +29,49 @@ test('it keeps equal objects in arrays', () => {
 })
 
 test('it handles changing cyclic structures', () => {
-  const cyclicPrev: Record<string, unknown> = {test: 'foo'}
-  cyclicPrev.self = cyclicPrev
+  const createObject = (differentiator: string) => {
+    // will be different if differentiator is different
+    const root: Record<string, any> = {id: 'root'}
 
-  const cyclicNext: Record<string, unknown> = {test: 'foo'}
-  cyclicNext.self = cyclicNext
+    // will be different if differentiator is different
+    root.a = {id: 'a'}
 
-  const prev = {arr: [{foo: 'bar'}], cyclic: cyclicPrev}
-  const next = {arr: [{foo: 'bar'}], cyclic: cyclicNext}
-  expect(immutableReconcile(prev, next).arr).toBe(prev.arr)
+    // will be different if differentiator is different
+    root.a.b = {id: 'b', diff: differentiator}
+
+    // cycle
+    root.a.b.a = root.a
+    // will never be different
+    root.a.b.c = {id: 'c'}
+
+    return root
+  }
+
+  const prev = createObject('previous')
+  const next = createObject('next')
 
-  // it picks from ´next´, so in this case even though the cyclicPrev and cyclicNext values are structurally equal it
-  // returns a different object identity for the `cyclic` key, since strictly speaking it has changed to *another* cyclic structure (which only incidentally is the same).
   const reconciled = immutableReconcile(prev, next)
-  expect(reconciled.cyclic).not.toBe(prev.cyclic)
-  expect(reconciled.cyclic.self).toBe(cyclicNext)
+
+  expect(prev).not.toBe(reconciled)
+  expect(next).not.toBe(reconciled)
+
+  // A sub object of root has changed, creating new object
+  expect(next.a).not.toBe(reconciled.a)
+
+  // A sub-object of root.a has changed, creating new object
+  expect(next.a.b).not.toBe(reconciled.a.b)
+
+  // root.a.b.c is has not changed, therefore reuse.
+  expect(next.a.b.c).not.toBe(reconciled.a.b.c)
+
+  expect(prev.a.b.c).toBe(reconciled.a.b.c)
+
+  // The new reconcile will retain reconcilable objects also within loops.
+  expect(prev.a.b.a.b.c).toBe(reconciled.a.b.a.b.c)
+
+  // This is because it retains the loop.
+  expect(reconciled.a).toBe(reconciled.a.b.a)
+  expect(prev.a.b.c).toBe(reconciled.a.b.a.b.c)
 })
 
 test('it handles non-changing cyclic structures', () => {

packages/sanity/src/core/form/store/utils/immutableReconcile.ts

@@ -5,7 +5,7 @@
  * @param next - the next/current value
  */
 export function immutableReconcile<T>(previous: unknown, next: T): T {
-  return _immutableReconcile(previous, next, new WeakSet())
+  return _immutableReconcile(previous, next, new WeakMap())
 }
 
 function _immutableReconcile<T>(
@@ -14,12 +14,12 @@ function _immutableReconcile<T>(
   /**
    * Keep track of visited nodes to prevent infinite recursion in case of circular structures
    */
-  parents: WeakSet<any>,
+  parents: WeakMap<any, any>,
 ): T {
   if (previous === next) return previous as T
 
   if (parents.has(next)) {
-    return next
+    return parents.get(next)
   }
 
   // eslint-disable-next-line no-eq-null
@@ -32,13 +32,12 @@ function _immutableReconcile<T>(
   if (prevType !== nextType) return next
 
   if (Array.isArray(next)) {
-    parents.add(next)
-
     assertType<unknown[]>(previous)
     assertType<unknown[]>(next)
 
     let allEqual = previous.length === next.length
-    const result = []
+    const result: unknown[] = []
+    parents.set(next, result)
     for (let index = 0; index < next.length; index++) {
       const nextItem = _immutableReconcile(previous[index], next[index], parents)
 
@@ -47,26 +46,26 @@ function _immutableReconcile<T>(
       }
       result[index] = nextItem
     }
-    parents.delete(next)
+    parents.set(next, allEqual ? previous : result)
     return (allEqual ? previous : result) as any
   }
 
   if (typeof next === 'object') {
-    parents.add(next)
     assertType<Record<string, unknown>>(previous)
     assertType<Record<string, unknown>>(next)
 
     const nextKeys = Object.keys(next)
     let allEqual = Object.keys(previous).length === nextKeys.length
     const result: Record<string, unknown> = {}
+    parents.set(next, result)
     for (const key of nextKeys) {
       const nextValue = _immutableReconcile(previous[key], next[key]!, parents)
       if (nextValue !== previous[key]) {
         allEqual = false
       }
       result[key] = nextValue
     }
-    parents.delete(next)
+    parents.set(next, allEqual ? previous : result)
     return (allEqual ? previous : result) as T
   }
   return next