Bug(reactivity): Fix collection-helper edge cases (#221)

Author: jlukic

packages/reactivity/src/reaction.js

@@ -69,7 +69,6 @@ export class Reaction {
   }
 
   run() {
-    // only run this reaction is marked as active
     if (!this.active) {
       return;
     }

packages/reactivity/src/scheduler.js

@@ -45,7 +45,7 @@ export class Scheduler {
           const toRun = Scheduler.pendingReactions;
           Scheduler.pendingReactions = new Set();
           for (const r of toRun) {
-            if (r.stopped) { continue; }
+            if (!r.active) { continue; }
             try {
               r.run();
             }

packages/reactivity/src/signal.js

@@ -29,12 +29,14 @@ export class Signal {
   // default clone and equal pulled from utils
   static equalityFunction = isEqual;
   static cloneFunction = clone;
+  static idFunction = (item) => item.id ?? item._id ?? item.hash ?? item.key;
   static safety = 'reference';
 
   constructor(initialValue, {
     safety = Signal.safety,
     cloneFunction = Signal.cloneFunction,
     equalityFunction = (safety === 'none') ? returnsFalse : Signal.equalityFunction,
+    idFunction = Signal.idFunction,
     context,
   } = {}) {
     // create dependency
@@ -46,6 +48,7 @@ export class Signal {
     // handle custom helpers if user specifies
     this.cloneFunction = cloneFunction;
     this.equalityFunction = equalityFunction;
+    this.idFunction = idFunction;
 
     this.safety = safety;
     this.currentValue = this.protect(initialValue);
@@ -321,13 +324,13 @@ export class Signal {
     if (!isObject(item)) {
       return [item];
     }
-    return unique([item.id, item._id, item.hash, item.key].filter(Boolean));
+    return unique([item.id, item._id, item.hash, item.key].filter(value => value != null));
   }
   getID(item) {
     if (!isObject(item)) {
       return item;
     }
-    return item.id || item._id || item.hash || item.key;
+    return this.idFunction(item);
   }
   hasID(item, id) {
     return this.getID(item) === id;
@@ -364,10 +367,18 @@ export class Signal {
     }
   }
   replaceItem(id, item) {
-    return this.setIndex(this.getItemIndex(id), item);
+    const index = this.getItemIndex(id);
+    if (index === -1) {
+      return;
+    }
+    return this.setIndex(index, item);
   }
   removeItem(id) {
-    return this.removeIndex(this.getItemIndex(id));
+    const index = this.getItemIndex(id);
+    if (index === -1) {
+      return;
+    }
+    return this.removeIndex(index);
   }
 
   /*******************************

packages/reactivity/test/unit/signal-helpers.test.js

@@ -341,6 +341,18 @@ SAFETY_MODES.forEach((safety) => {
         const sig = new Signal([], { safety });
         expect(sig.getID('plain')).toBe('plain');
       });
+
+      it('treats a falsy-but-present id as the id', () => {
+        const sig = new Signal([], { safety });
+        expect(sig.getID({ id: 0 })).toBe(0);
+        expect(sig.getID({ id: '' })).toBe('');
+      });
+
+      it('falls through fields only when the prior is null or undefined', () => {
+        const sig = new Signal([], { safety });
+        expect(sig.getID({ id: 0, _id: 'b' })).toBe(0);
+        expect(sig.getID({ id: null, _id: 'b' })).toBe('b');
+      });
     });
 
     describe('getIDs', () => {
@@ -356,6 +368,11 @@ SAFETY_MODES.forEach((safety) => {
         const sig = new Signal([], { safety });
         expect(sig.getIDs('plain')).toEqual(['plain']);
       });
+
+      it('keeps a zero id rather than dropping it', () => {
+        const sig = new Signal([], { safety });
+        expect(sig.getIDs({ id: 0 })).toEqual([0]);
+      });
     });
 
     describe('hasID', () => {
@@ -591,6 +608,19 @@ SAFETY_MODES.forEach((safety) => {
         sig.replaceItem('a', { id: 'a', v: 100 });
         expect(sig.raw()).toBe(before);
       });
+
+      it('is a no-op when no item matches the id', () => {
+        const sig = new Signal(
+          [{ id: 'a', v: 1 }, { id: 'b', v: 2 }],
+          { safety },
+        );
+        const sub = subscribe(sig);
+        sig.replaceItem('missing', { id: 'missing', v: 99 });
+        Reaction.flush();
+        expect(sig.raw()).toEqual([{ id: 'a', v: 1 }, { id: 'b', v: 2 }]);
+        expect(sub.count).toBe(0);
+        sub.stop();
+      });
     });
 
     describe('removeItem', () => {
@@ -624,6 +654,19 @@ SAFETY_MODES.forEach((safety) => {
         sig.removeItem('b');
         expect(sig.raw()).toBe(before);
       });
+
+      it('is a no-op when no item matches the id (does not delete the last item)', () => {
+        const sig = new Signal(
+          [{ id: 'a' }, { id: 'b' }, { id: 'c' }],
+          { safety },
+        );
+        const sub = subscribe(sig);
+        sig.removeItem('missing');
+        Reaction.flush();
+        expect(sig.raw().map(i => i.id)).toEqual(['a', 'b', 'c']);
+        expect(sub.count).toBe(0);
+        sub.stop();
+      });
     });
 
     /*******************************

packages/reactivity/test/unit/signal.test.js

@@ -753,6 +753,27 @@ describe('Signal API', () => {
       expect(cloneSpy).toHaveBeenCalled();
       expect(signal.peek().cloned).toBe(true);
     });
+
+    it('default idFunction prefers id over _id', () => {
+      const signal = new Signal([]);
+      expect(signal.getID({ _id: 'mongo', id: 'app' })).toBe('app');
+    });
+
+    it('uses a per-instance idFunction override for getID', () => {
+      const signal = new Signal([], { idFunction: item => item.slug });
+      expect(signal.getID({ slug: 'x', id: 'y' })).toBe('x');
+    });
+
+    it('falls back to the static Signal.idFunction when no override is given', () => {
+      const original = Signal.idFunction;
+      Signal.idFunction = item => item.slug;
+      try {
+        expect(new Signal([]).getID({ slug: 'x', id: 'y' })).toBe('x');
+      }
+      finally {
+        Signal.idFunction = original;
+      }
+    });
   });
 
   /***********************************************

packages/renderer/src/shared/each.js

@@ -3,6 +3,7 @@
   pure logic, no DOM or engine primitives
 */
 
+import { Signal } from '@semantic-ui/reactivity';
 import { isArray, isPlainObject, isString } from '@semantic-ui/utils';
 
 export function getCollectionType(items) {
@@ -15,7 +16,9 @@ export function getItemID(item, indexOrKey, collectionType) {
   let raw;
   if (isPlainObject(item)) {
     const key = (collectionType === 'object') ? indexOrKey : undefined;
-    raw = key || item._id || item.id || item.key || item.hash || item._hash || item.value || indexOrKey;
+    // key is the object positional index — a 0 falls through (||) to the
+    // shared id resolver, so each + signals key items the same way
+    raw = key || (Signal.idFunction(item) ?? indexOrKey);
   }
   else if (isString(item)) {
     raw = item + ':' + indexOrKey;