refactor: simplify cache usage for object cycles (#77)

* add test with object cycles * simplify cache usage * should not reach there
dai-shi · Nov 22, 2024 · 5a9adac · 5a9adac
1 parent 1c0f138
commit 5a9adac
Show file tree

Hide file tree

Showing 2 changed files with 75 additions and 51 deletions.
diff --git a/src/index.ts b/src/index.ts
@@ -10,8 +10,6 @@ const IS_TARGET_COPIED_PROPERTY = 'f';
 const PROXY_PROPERTY = 'p';
 const PROXY_CACHE_PROPERTY = 'c';
 const TARGET_CACHE_PROPERTY = 't';
-const NEXT_OBJECT_PROPERTY = 'n';
-const CHANGED_PROPERTY = 'g';
 const HAS_KEY_PROPERTY = 'h';
 const ALL_OWN_KEYS_PROPERTY = 'w';
 const HAS_OWN_KEY_PROPERTY = 'o';
@@ -252,14 +250,6 @@ const isAllOwnKeysChanged = (prevObj: object, nextObj: object) => {
   );
 };
 
-type ChangedCache = WeakMap<
-  object,
-  {
-    [NEXT_OBJECT_PROPERTY]: object;
-    [CHANGED_PROPERTY]: boolean;
-  }
->;
-
 /**
  * Compare changes on objects.
  *
@@ -299,7 +289,7 @@ export const isChanged = (
   prevObj: unknown,
   nextObj: unknown,
   affected: WeakMap<object, unknown>,
-  cache?: WeakMap<object, unknown>,
+  cache?: WeakMap<object, unknown>, // for object with cycles
   isEqual: (a: unknown, b: unknown) => boolean = Object.is,
 ): boolean => {
   if (isEqual(prevObj, nextObj)) {
@@ -309,53 +299,41 @@ export const isChanged = (
   const used = (affected as Affected).get(getOriginalObject(prevObj));
   if (!used) return true;
   if (cache) {
-    const hit = (cache as ChangedCache).get(prevObj);
-    if (hit && hit[NEXT_OBJECT_PROPERTY] === nextObj) {
-      return hit[CHANGED_PROPERTY];
+    const hit = cache.get(prevObj);
+    if (hit === nextObj) {
+      return false;
     }
     // for object with cycles
-    (cache as ChangedCache).set(prevObj, {
-      [NEXT_OBJECT_PROPERTY]: nextObj,
-      [CHANGED_PROPERTY]: false,
-    });
+    cache.set(prevObj, nextObj);
   }
   let changed: boolean | null = null;
-  try {
-    for (const key of used[HAS_KEY_PROPERTY] || []) {
-      changed = Reflect.has(prevObj, key) !== Reflect.has(nextObj, key);
-      if (changed) return changed;
-    }
-    if (used[ALL_OWN_KEYS_PROPERTY] === true) {
-      changed = isAllOwnKeysChanged(prevObj, nextObj);
-      if (changed) return changed;
-    } else {
-      for (const key of used[HAS_OWN_KEY_PROPERTY] || []) {
-        const hasPrev = !!Reflect.getOwnPropertyDescriptor(prevObj, key);
-        const hasNext = !!Reflect.getOwnPropertyDescriptor(nextObj, key);
-        changed = hasPrev !== hasNext;
-        if (changed) return changed;
-      }
-    }
-    for (const key of used[KEYS_PROPERTY] || []) {
-      changed = isChanged(
-        (prevObj as any)[key],
-        (nextObj as any)[key],
-        affected,
-        cache,
-        isEqual,
-      );
+  for (const key of used[HAS_KEY_PROPERTY] || []) {
+    changed = Reflect.has(prevObj, key) !== Reflect.has(nextObj, key);
+    if (changed) return changed;
+  }
+  if (used[ALL_OWN_KEYS_PROPERTY] === true) {
+    changed = isAllOwnKeysChanged(prevObj, nextObj);
+    if (changed) return changed;
+  } else {
+    for (const key of used[HAS_OWN_KEY_PROPERTY] || []) {
+      const hasPrev = !!Reflect.getOwnPropertyDescriptor(prevObj, key);
+      const hasNext = !!Reflect.getOwnPropertyDescriptor(nextObj, key);
+      changed = hasPrev !== hasNext;
       if (changed) return changed;
     }
-    if (changed === null) changed = true;
-    return changed;
-  } finally {
-    if (cache) {
-      cache.set(prevObj, {
-        [NEXT_OBJECT_PROPERTY]: nextObj,
-        [CHANGED_PROPERTY]: changed,
-      });
-    }
   }
+  for (const key of used[KEYS_PROPERTY] || []) {
+    changed = isChanged(
+      (prevObj as any)[key],
+      (nextObj as any)[key],
+      affected,
+      cache,
+      isEqual,
+    );
+    if (changed) return changed;
+  }
+  if (changed === null) throw new Error('invalid used');
+  return changed;
 };
 
 // explicitly track object with memo

diff --git a/tests/17_cycles.spec.ts b/tests/17_cycles.spec.ts
@@ -0,0 +1,46 @@
+import { describe, expect, it } from 'vitest';
+import { createProxy, isChanged } from 'proxy-compare';
+
+const noop = (_arg: unknown) => {
+  // do nothing
+};
+
+describe('object with cycles', () => {
+  interface S {
+    a: string;
+    s?: S;
+  }
+
+  it('without cache', () => {
+    const s1: S = { a: 'a' };
+    s1.s = s1;
+    const s2: S = { a: 'a' };
+    s2.s = s2;
+    const a1 = new WeakMap();
+    const p1 = createProxy(s1, a1);
+    noop(p1.s?.a);
+    expect(() => isChanged(s1, s2, a1)).toThrow();
+  });
+
+  it('with cache', () => {
+    const s1: S = { a: 'a' };
+    s1.s = s1;
+    const s2: S = { a: 'a' };
+    s2.s = s2;
+    const a1 = new WeakMap();
+    const p1 = createProxy(s1, a1);
+    noop(p1.s?.a);
+    expect(isChanged(s1, s2, a1, new WeakMap())).toBe(false);
+  });
+
+  it('with cache with a change', () => {
+    const s1: S = { a: 'a' };
+    s1.s = s1;
+    const s2: S = { a: 'aa' };
+    s2.s = s2;
+    const a1 = new WeakMap();
+    const p1 = createProxy(s1, a1);
+    noop(p1.s?.a);
+    expect(isChanged(s1, s2, a1, new WeakMap())).toBe(true);
+  });
+});