arrayChange supports recursive updates--modifying array within the ar…

…rayChange callback; fixes knockout#1552
smilce · Nov 6, 2014 · a877d8e · a877d8e
1 parent a88830e
commit a877d8e
Show file tree

Hide file tree

Showing 2 changed files with 48 additions and 6 deletions.
diff --git a/spec/observableArrayChangeTrackingBehaviors.js b/spec/observableArrayChangeTrackingBehaviors.js
@@ -19,17 +19,22 @@ describe('Observable Array change tracking', function() {
 
     it('Only computes diffs when there\'s at least one active arrayChange subscription', function() {
         captureCompareArraysCalls(function(callLog) {
-            var myArray = ko.observableArray(['Alpha', 'Beta', 'Gamma']);
+            var myArray = ko.observableArray(['Alpha', 'Beta', 'Gamma']),
+                changelist;
 
             // Nobody has yet subscribed for arrayChange notifications, so
             // array mutations don't involve computing diffs
             myArray(['Another']);
             expect(callLog.length).toBe(0);
 
             // When there's a subscriber, it does compute diffs
-            var subscription = myArray.subscribe(function() {}, null, 'arrayChange');
+            var subscription = myArray.subscribe(function(changes) { changelist = changes; }, null, 'arrayChange');
             myArray(['Changed']);
             expect(callLog.length).toBe(1);
+            expect(changelist).toEqual([
+                { status: 'deleted', value: 'Another', index: 0 },
+                { status: 'added', value: 'Changed', index: 0 }
+            ]);
 
             // If all the subscriptions are disposed, it stops computing diffs
             subscription.dispose();
@@ -38,9 +43,14 @@ describe('Observable Array change tracking', function() {
 
             // ... but that doesn't stop someone else subscribing in the future,
             // then diffs are computed again
-            myArray.subscribe(function() {}, null, 'arrayChange');
+            myArray.subscribe(function(changes) { changelist = changes; }, null, 'arrayChange');
             myArray(['Changed once more']);
             expect(callLog.length).toBe(2);
+            // Verify that changes are from the previous array value (at subscription time) and not from the last notified value
+            expect(changelist).toEqual([
+                { status: 'deleted', value: 'Changed again', index: 0 },
+                { status: 'added', value: 'Changed once more', index: 0 }
+            ]);
         });
     });
 
@@ -269,6 +279,37 @@ describe('Observable Array change tracking', function() {
         ]);
     });
 
+    it('Should support recursive updates (modify array within arrayChange callback)', function() {
+        // See https://github.com/knockout/knockout/issues/1552
+        var toAdd = {
+            name: "1",
+            nodes: [
+                { name: "1.1", nodes: [ { name: "1.1.1", nodes: [] } ] },
+                { name: "1.2", nodes: [] },
+                { name: "1.3", nodes: [] }
+            ]
+        };
+        var list = ko.observableArray([]);
+
+        // This adds all descendent nodes to the list when a node is added
+        list.subscribe(function (events) {
+            events = events.slice(0);
+            for (var i = 0; i < events.length; i++) {
+                var event = events[i];
+                switch (event.status) {
+                    case "added":
+                        list.push.apply(list, event.value.nodes);
+                    break;
+                }
+            }
+        }, null, "arrayChange");
+
+        // Add the top-level node
+        list.push(toAdd);
+        // See that descendent nodes are also added
+        expect(list()).toEqual([ toAdd, toAdd.nodes[0], toAdd.nodes[1], toAdd.nodes[2], toAdd.nodes[0].nodes[0] ]);
+    });
+
     function testKnownOperation(array, operationName, options) {
         var changeList,
             subscription = array.subscribe(function(changes) {

diff --git a/src/subscribables/observableArray.changeTracking.js b/src/subscribables/observableArray.changeTracking.js
@@ -45,15 +45,16 @@ ko.extenders['trackArrayChanges'] = function(target) {
             // Compute the diff and issue notifications, but only if someone is listening
             if (target.hasSubscriptionsForEvent(arrayChangeEventName)) {
                 var changes = getChanges(previousContents, currentContents);
-                if (changes.length) {
-                    target['notifySubscribers'](changes, arrayChangeEventName);
-                }
             }
 
             // Eliminate references to the old, removed items, so they can be GCed
             previousContents = currentContents;
             cachedDiff = null;
             pendingNotifications = 0;
+
+            if (changes && changes.length) {
+                target['notifySubscribers'](changes, arrayChangeEventName);
+            }
         });
     }