fix changing an observable value from an onChange immediate updating …

…the final batch correctly, and removing from batch if unchanged
LegendApp · Jun 10, 2024 · 45d35a4 · 45d35a4
1 parent 74a3242
commit 45d35a4
Show file tree

Hide file tree

Showing 2 changed files with 51 additions and 4 deletions.
diff --git a/src/batching.ts b/src/batching.ts
@@ -80,14 +80,17 @@ export function notify(node: NodeValue, value: any, prev: any, level: number, wh
         level,
         whenOptimizedOnlyIf,
     );
-    if (changesInBatch.size) {
-        batchNotifyChanges(changesInBatch, /*immediate*/ true);
-    }
 
     // Update the current batch
     const existing = _batchMap.get(node);
     if (existing) {
-        existing.value = value;
+        // Check that value is still different from prev as it could have been reverted during the batch,
+        // then don't need to notify for it
+        if (existing.prev === value) {
+            _batchMap.delete(node);
+        } else {
+            existing.value = value;
+        }
         // TODO: level, whenOptimizedOnlyIf
     } else {
         _batchMap.set(node, {
@@ -100,6 +103,10 @@ export function notify(node: NodeValue, value: any, prev: any, level: number, wh
         });
     }
 
+    if (changesInBatch.size) {
+        batchNotifyChanges(changesInBatch, /*immediate*/ true);
+    }
+
     // If not in a batch run it immediately
     if (numInBatch <= 0) {
         runBatch();

diff --git a/tests/tests.test.ts b/tests/tests.test.ts
@@ -3452,3 +3452,43 @@ describe('Dates', () => {
         expect(handler2).not.toHaveBeenCalled();
     });
 });
+describe('Middleware', () => {
+    test('use onChange immediate to revert', () => {
+        const obs$ = observable({ text: 'hi' });
+        obs$.onChange(
+            ({ value, getPrevious }) => {
+                if (value.text === 'bad') {
+                    obs$.text.set(getPrevious().text);
+                }
+            },
+            { immediate: true },
+        );
+
+        const handler = expectChangeHandler(obs$);
+
+        obs$.text.set('bad');
+
+        expect(handler).not.toHaveBeenCalled();
+        expect(obs$.text.get()).toEqual('hi');
+    });
+    test('use onChange immediate to change', () => {
+        const obs$ = observable({ text: 'hi' });
+        obs$.onChange(
+            ({ value }) => {
+                if (value.text === 'bad') {
+                    obs$.text.set('good');
+                }
+            },
+            { immediate: true },
+        );
+
+        const handler = expectChangeHandler(obs$);
+
+        obs$.text.set('bad');
+
+        expect(handler).toHaveBeenCalledWith({ text: 'good' }, { text: 'hi' }, [
+            { path: ['text'], pathTypes: ['object'], prevAtPath: 'hi', valueAtPath: 'good' },
+        ]);
+        expect(obs$.text.get()).toEqual('good');
+    });
+});