Fixes for ReactiveSet (#688)

Author: thetarnav

.changeset/long-squids-report.md

@@ -0,0 +1,9 @@
+---
+"@solid-primitives/set": minor
+---
+
+Fixes for ReactiveSet (#688):
+- Iterators and `.forEach()` no longer track specific keys.
+- Added support for `thisArg` as per `forEach` spec
+- `super.clear()` now called before dirtying signals
+- Uses new `dirtyAll` form trigger package

.changeset/swift-points-dance.md

@@ -0,0 +1,5 @@
+---
+"@solid-primitives/trigger": minor
+---
+
+Add `dirtyAll` to `createTriggerCache`

packages/set/src/index.ts

@@ -23,69 +23,76 @@ export class ReactiveSet<T> extends Set<T> {
     if (values) for (const v of values) super.add(v);
   }
 
-  // reads
+  [Symbol.iterator](): SetIterator<T> {
+    return this.values();
+  }
+
   get size(): number {
     this.#triggers.track($KEYS);
     return super.size;
   }
-  has(v: T): boolean {
-    this.#triggers.track(v);
-    return super.has(v);
+
+  has(value: T): boolean {
+    this.#triggers.track(value);
+    return super.has(value);
   }
 
-  *keys(): SetIterator<T> {
-    for (const key of super.keys()) {
-      this.#triggers.track(key);
-      yield key;
-    }
-    this.#triggers.track($KEYS);
+  keys(): SetIterator<T> {
+    return this.values();
   }
-  values(): SetIterator<T> {
-    return this.keys();
+
+  *values(): SetIterator<T> {
+    this.#triggers.track($KEYS);
+
+    for (const value of super.values()) {
+      yield value;
+    }
   }
+
   *entries(): SetIterator<[T, T]> {
-    for (const key of super.keys()) {
-      this.#triggers.track(key);
-      yield [key, key];
-    }
     this.#triggers.track($KEYS);
-  }
 
-  [Symbol.iterator](): SetIterator<T> {
-    return this.values();
+    for (const entry of super.entries()) {
+      yield entry;
+    }
   }
-  forEach(callbackfn: (value: T, value2: T, set: this) => void) {
+
+  forEach(callbackfn: (value1: T, value2: T, set: Set<T>) => void, thisArg?: any): void {
     this.#triggers.track($KEYS);
-    super.forEach(callbackfn as any);
+    super.forEach(callbackfn, thisArg);
   }
 
-  // writes
-  add(v: T): this {
-    if (!super.has(v)) {
-      super.add(v);
+  add(value: T): this {
+    if (!super.has(value)) {
+      super.add(value);
       batch(() => {
-        this.#triggers.dirty(v);
+        this.#triggers.dirty(value);
         this.#triggers.dirty($KEYS);
       });
     }
+
     return this;
   }
-  delete(v: T): boolean {
-    const r = super.delete(v);
-    if (r) {
+
+  delete(value: T): boolean {
+    const result = super.delete(value);
+
+    if (result) {
       batch(() => {
-        this.#triggers.dirty(v);
+        this.#triggers.dirty(value);
         this.#triggers.dirty($KEYS);
       });
     }
-    return r;
+
+    return result;
   }
+
   clear(): void {
     if (super.size) {
+      super.clear();
+
       batch(() => {
-        for (const v of super.keys()) this.#triggers.dirty(v);
-        super.clear();
-        this.#triggers.dirty($KEYS);
+        this.#triggers.dirtyAll();
       });
     }
   }
@@ -109,21 +116,25 @@ export class ReactiveWeakSet<T extends object> extends WeakSet<T> {
     if (values) for (const v of values) super.add(v);
   }
 
-  has(v: T): boolean {
-    this.#triggers.track(v);
-    return super.has(v);
+  has(value: T): boolean {
+    this.#triggers.track(value);
+    return super.has(value);
   }
-  add(v: T): this {
-    if (!super.has(v)) {
-      super.add(v);
-      this.#triggers.dirty(v);
+
+  add(value: T): this {
+    if (!super.has(value)) {
+      super.add(value);
+      this.#triggers.dirty(value);
     }
     return this;
   }
-  delete(v: T): boolean {
-    const deleted = super.delete(v);
-    deleted && this.#triggers.dirty(v);
-    return deleted;
+
+  delete(value: T): boolean {
+    const result = super.delete(value);
+
+    result && this.#triggers.dirty(value);
+
+    return result;
   }
 }
 

packages/set/test/index.test.ts

@@ -100,33 +100,39 @@ describe("ReactiveSet", () => {
       const dispose = createRoot(dispose => {
         createEffect(() => {
           const run: number[] = [];
-          let i = 0;
           for (const key of fn(set)) {
             run.push(key);
-            if (i === 2) break; // don't iterate over all keys
-            i += 1;
           }
           captured.push(run);
         });
         return dispose;
       });
 
       expect(captured).toHaveLength(1);
-      expect(captured[0]).toEqual([1, 2, 3]);
+      expect(captured[0]).toEqual([1, 2, 3, 4]);
 
       set.delete(4);
-      expect(captured, "deleted unseen key").toHaveLength(1);
+      expect(captured).toHaveLength(2);
+      expect(captured[1]).toEqual([1, 2, 3]);
 
       set.delete(1);
-      expect(captured, "deleted seen").toHaveLength(2);
-      expect(captured[1]).toEqual([2, 3]);
+      expect(captured).toHaveLength(3);
+      expect(captured[2]).toEqual([2, 3]);
 
       set.add(4);
-      expect(captured, "added key in reach").toHaveLength(3);
-      expect(captured[2]).toEqual([2, 3, 4]);
+      expect(captured).toHaveLength(4);
+      expect(captured[3]).toEqual([2, 3, 4]);
 
       set.add(5);
-      expect(captured, "added key out of reach").toHaveLength(3);
+      expect(captured).toHaveLength(5);
+      expect(captured[4]).toEqual([2, 3, 4, 5]);
+
+      set.add(5);
+      expect(captured).toHaveLength(5);
+
+      set.clear();
+      expect(captured).toHaveLength(6);
+      expect(captured[5]).toEqual([]);
 
       dispose();
     });

packages/trigger/README.md

@@ -75,6 +75,22 @@ map.dirty(1);
 map.dirty(2);
 ```
 
+### Triggering all keys
+
+`dirtyAll` will trigger all keys in the cache.
+
+```ts
+const [track, _dirty, dirtyAll] = createTriggerCache<number>();
+
+createEffect(() => {
+  track(1);
+  //  ...
+});
+
+// later (will trigger the update)
+dirtyAll();
+```
+
 ### Weak version
 
 `createTriggerCache` constructor can take a `WeakMap` constructor as an argument. This will create a `WeakMap` of triggers instead of a `Map`.

packages/trigger/src/index.ts

@@ -50,6 +50,11 @@ export class TriggerCache<T> {
     this.#map.get(key)?.$$();
   }
 
+  dirtyAll() {
+    if (isServer) return;
+    for (const trigger of this.#map.values()) trigger.$$();
+  }
+
   track(key: T) {
     if (!getListener()) return;
     let trigger = this.#map.get(key);
@@ -77,11 +82,12 @@ export class TriggerCache<T> {
  * Trigger signals added to the cache only when tracked under a computation,
  * and get deleted from the cache when they are no longer tracked.
  *
- * @returns a tuple of `[track, dirty]` functions
+ * @returns a tuple of `[track, dirty, dirtyAll]` functions
  *
  * `track` and `dirty` are called with a `key` so that each tracker will trigger an update only when his individual `key` would get marked as dirty.
+ * `dirtyAll` will mark all keys as dirty and trigger an update for all of them.
  * @example
- * const [track, dirty] = createTriggerCache()
+ * const [track, dirty, dirtyAll] = createTriggerCache()
  * createEffect(() => {
  *    track(1)
  *    ...
@@ -90,10 +96,12 @@ export class TriggerCache<T> {
  * dirty(1)
  * // this won't cause an update:
  * dirty(2)
+ * // this will cause an update to all keys:
+ * dirtyAll()
  */
 export function createTriggerCache<T>(
   mapConstructor: WeakMapConstructor | MapConstructor = Map,
-): [track: (key: T) => void, dirty: (key: T) => void] {
+): [track: (key: T) => void, dirty: (key: T) => void, dirtyAll: () => void] {
   const map = new TriggerCache(mapConstructor);
-  return [map.track.bind(map), map.dirty.bind(map)];
+  return [map.track.bind(map), map.dirty.bind(map), map.dirtyAll.bind(map)];
 }

packages/trigger/test/index.test.ts

@@ -22,6 +22,21 @@ describe("createTriggerCache", () => {
       dispose();
     }));
 
+    test("dirtyAll", () =>
+      createRoot(dispose => {
+        const [track, , dirtyAll] = createTriggerCache(Map);
+        let runs = -1;
+        createComputed(() => {
+          track(1);
+          runs++;
+        });
+        expect(runs).toBe(0);
+        dirtyAll();
+        expect(runs).toBe(1);
+
+        dispose();
+      }));
+
   test("weak trigger cache", () =>
     createRoot(dispose => {
       const [track, dirty] = createTriggerCache();