feat(watch_group): split group manipulation into newGroup and addGroup

Previously, creating a new watch group simultaneously attached it to the parent from which it was created. Now, we have split this behavior into two steps so that it is possible to create a watch group, set up watches and then choose which parent to attach it to at a later point in time. You can also remove a watch group and attach it to a different parent. These features allow a more elegant mechanism for turning watches on/off based on whether or not the associated view is attached to the DOM. A primary use case for this is to tie into the attachedCallback/detachedCallback of the custom element lifecycle in order to better manage databinding present in element templates.

Closes issue #32

Author: EisenbergEffect

src/watch_group.js

@@ -32,17 +32,14 @@ function putIfAbsent(obj, key, ctor) {
 }
 
 export class WatchGroup {
-  constructor(parentWatchGroup, getterCache, context, cache, rootGroup) {
-    // TODO: Traceur Assertions
-    // assert(parentWatchGroup is WatchGroup)
-    // assert(changeDetector is ChangeDetector)
-    // assert(context and context is Function or Object)
-    // assert(rootGroup is RootWatchGroup)
-    this._parentWatchGroup = parentWatchGroup;
+  constructor(id, getterCache, context, cache, rootGroup) {
+    this.id = id;
+
     // Initialize _WatchGroupList
     this._watchGroupHead = this._watchGroupTail = null;
     this._nextWatchGroup = this._prevWatchGroup = null;
-    this.id = `${parentWatchGroup.id}.${parentWatchGroup._nextChildId++}`;
+    this._dirtyWatchHead = this._dirtyWatchTail = null;
+
     this._getterCache = getterCache;
     this.context = context;
     this._cache = cache;
@@ -54,8 +51,7 @@ export class WatchGroup {
     this._evalWatchHead = this._evalWatchTail = this._marker;
 
     this._dirtyMarker = ChangeRecord.marker();
-    this._recordTail = this._parentWatchGroup._childInclRecordTail;
-    this._recordHead = this._recordTail = this._recordAdd(this._dirtyMarker);
+    this._recordHead = this._recordTail = this._dirtyMarker;
 
     // Stats...
     this.fieldCost = 0; // Stats: Number of field watchers which are in use
@@ -179,51 +175,66 @@ export class WatchGroup {
   // [context]. If not present than child expressions will evaluate on same context allowing
   // the reuse of the expression cache.
   newGroup(context) {
-    var prev = this._childWatchGroupTail._evalWatchTail;
-    var next = prev._nextEvalWatch;
-
     if (arguments.length === 0 || context === null) {
       context = this.context;
     }
 
     var root = this._rootGroup === null ? this : this._rootGroup;
+    var id = `${this.id}.${this._nextChildId++}`;
     var cache = context === null ? this._cache : {};
 
-    var childGroup = new WatchGroup(this, this._getterCache, context, cache, root);
+    var childGroup = new WatchGroup(id, this._getterCache, context, cache, root);
+
+    return childGroup;
+  }
+
+  addGroup(childGroup){
+    childGroup.id = `${this.id}.${this._nextChildId++}`;
+    childGroup._parentWatchGroup = this;
+
+    var prevEval = this._childWatchGroupTail._evalWatchTail;
+    var nextEval = prevEval._nextEvalWatch;
+    var prevRecord = this._childWatchGroupTail._recordTail;
+    var nextRecord = prevRecord.nextRecord;
+
     _WatchGroupList._add(this, childGroup);
 
-    var marker = childGroup._marker;
+    var evalMarker = childGroup._marker;
 
-    marker._prevEvalWatch = prev;
-    marker._nextEvalWatch = next;
+    evalMarker._prevEvalWatch = prevEval;
+    evalMarker._nextEvalWatch = nextEval;
 
-    if (prev !== null) prev._nextEvalWatch = marker;
-    if (next !== null) next._prevEvalWatch = marker;
+    if (prevEval !== null) prevEval._nextEvalWatch = evalMarker;
+    if (nextEval !== null) nextEval._prevEvalWatch = evalMarker;
 
-    return childGroup;
+    var childRecordHead = childGroup._recordHead;
+    var childRecordTail = childGroup._recordTail;
+
+    childRecordHead.prevRecord = prevRecord;
+    childRecordTail.nextRecord = nextRecord;
+
+    if (prevRecord !== null) prevRecord.nextRecord = childRecordHead;
+    if (nextRecord !== null) nextRecord.prevRecord = childRecordTail;
+
+    // TODO:(eisenbergeffect) attach observers associated with dirty records?
   }
 
-  // Remove/destroy [WatchGroup] and all of its watches
   remove() {
-    // TODO:(misko) This code is not right.
-    // 1) It fails to release [ChangeDetector] [WatchRecord]s
+    // TODO:(eisenbergeffect) detach observers associated with dirty records?
 
     var prevRecord = this._recordHead.prevRecord;
     var nextRecord = this._childInclRecordTail.nextRecord;
 
     if (prevRecord !== null) prevRecord.nextRecord = nextRecord;
     if (nextRecord !== null) nextRecord.prevRecord = prevRecord;
 
+    // TODO:(eisenbergeffect) investigate these two lines; not sure such properties exist on records
     this._recordHead._prevWatchGroup = null;
     this._recordTail._prevWatchGroup = null;
-    this._recordHead = this._recordTail = null;
-
 
     _WatchGroupList._remove(this._parentWatchGroup, this);
     this._nextWatchGroup = this._prevWatchGroup = null;
 
-    //TODO: this._changeDetector.remove();
-
     this._rootGroup._removeCount++;
     this._parentWatchGroup = null;
 
@@ -616,3 +627,4 @@ export class RootWatchGroup extends WatchGroup {
     return watch;
   }
 }
+

test/dirtychecking.spec.js

@@ -132,9 +132,9 @@ describe('DirtyCheckingChangeDetector', function() {
       setup();
       var obj = {};
       var ra = watchGrp.watchField(obj, 'a', '0a');
-      var child1a = watchGrp.newGroup();
-      var child1b = watchGrp.newGroup();
-      var child2 = child1a.newGroup();
+      var child1a = createAndAddGroup(watchGrp);
+      var child1b = createAndAddGroup(watchGrp);
+      var child2 = createAndAddGroup(child1a);
 
       child1a.watchField(obj, 'a', '1a');
       child1b.watchField(obj, 'a', '1b');
@@ -153,7 +153,7 @@ describe('DirtyCheckingChangeDetector', function() {
       setup();
       var obj = {};
       var ra = watchGrp.watchField(obj, 'a', 'a');
-      var child = watchGrp.newGroup();
+      var child = createAndAddGroup(watchGrp);
       var cb = child.watchField(obj, 'b', 'b');
 
       obj['a'] = obj['b'] = 1;
@@ -177,9 +177,9 @@ describe('DirtyCheckingChangeDetector', function() {
 
     it('should properly add children', function() {
       setup();
-      var a = watchGrp.newGroup();
-      var aChild = a.newGroup();
-      var b = watchGrp.newGroup();
+      var a = createAndAddGroup(watchGrp);
+      var aChild = createAndAddGroup(a);
+      var b = createAndAddGroup(watchGrp);
       expect(function() {
         watchGrp.collectChanges();
       }).not.toThrow();
@@ -580,3 +580,10 @@ class _User {
     this.age = age;
   }
 }
+
+function createAndAddGroup(parentGroup, context){
+  var childGroup = parentGroup.newGroup(context);
+  parentGroup.addGroup(childGroup);
+  return childGroup;
+}
+

test/observer.spec.js

@@ -123,7 +123,7 @@ describe('observer', function() {
           group;
 
       setup(observer);
-      group = watchGrp.newGroup();
+      group = createAndAddGroup(watchGrp);
       group.watchField(user, 'name', null);
 
       expect(observer.closeCalls).toBe(0);
@@ -183,3 +183,10 @@ class ExplicitObserver{
     this.callback = null;
   }
 }
+
+function createAndAddGroup(parentGroup, context){
+  var childGroup = parentGroup.newGroup(context);
+  parentGroup.addGroup(childGroup);
+  return childGroup;
+}
+

test/watchgroup.spec.js

@@ -313,7 +313,7 @@ describe('WatchGroup', function() {
 
     it('should react when pure function return value changes in child watchGroup', function() {
       setup();
-      var childWatchGrp = watchGrp.newGroup({'a': 1, 'b': 2});
+      var childWatchGrp = createAndAddGroup(watchGrp, {'a': 1, 'b': 2});
 
       var watch = childWatchGrp.watchExpression(new PureFunctionAST('add', function(a, b) {
         logger.log('+');
@@ -707,9 +707,9 @@ describe('WatchGroup', function() {
         proxy1 = Object.create(context);
         proxy2 = Object.create(context);
         proxy3 = Object.create(context);
-        child1a = watchGrp.newGroup(proxy1);
-        child1b = watchGrp.newGroup(proxy2);
-        child2 = child1a.newGroup(proxy3);
+        child1a = createAndAddGroup(watchGrp, proxy1);
+        child1b = createAndAddGroup(watchGrp, proxy2);
+        child2 = createAndAddGroup(child1a, proxy3);
 
         child1a.watchExpression(parse('a'), logValue('1a'));
         child1b.watchExpression(parse('a'), logValue('1b'));
@@ -727,6 +727,98 @@ describe('WatchGroup', function() {
         proxy1 = proxy2 = proxy3 = child1a = child1b = child2 = null;
       });
 
+      it('should be able to add watches without being attached to a parent', () => {
+        var changes = 0,
+            context = {a:'something'};
+
+        setup();
+        child1a = watchGrp.newGroup(context);
+        child1a.watchExpression(parse('a'), logValue('1a'));
+
+        context.a = 'another';
+        watchGrp.detectChanges(null, () => changes++);
+
+        expect(child1a.fieldCost).toBe(1);
+        expect(watchGrp.totalFieldCost).toBe(0);
+        expect(changes).toBe(0);
+        expect(`${logger}`).toBe('1a');
+      });
+
+      it('should be able to connect a group with existing watches', () => {
+        var changes = 0,
+            context = {a:'something'};
+
+        setup();
+        child1a = watchGrp.newGroup(context);
+        child1a.watchExpression(parse('a'), logValue('1a'));
+        watchGrp.addGroup(child1a);
+
+        context.a = 'another';
+        watchGrp.detectChanges(null, () => changes++);
+
+        expect(child1a.fieldCost).toBe(1);
+        expect(watchGrp.totalFieldCost).toBe(1);
+        expect(changes).toBe(1);
+        expect(`${logger}`).toBe('1a');
+      });
+
+      it('should be able to re-connect a group with existing watches', () => {
+        var changes = 0,
+            context = {a:'something'};
+
+        setup();
+        child1a = watchGrp.newGroup(context);
+        child1a.watchExpression(parse('a'), logValue('1a'));
+
+        watchGrp.addGroup(child1a);
+        child1a.remove();
+
+        context.a = 'another';
+        watchGrp.detectChanges(null, () => changes++);
+
+        expect(child1a.fieldCost).toBe(1);
+        expect(watchGrp.totalFieldCost).toBe(0);
+        expect(changes).toBe(0);
+
+        watchGrp.addGroup(child1a);
+
+        context.a = 'another2';
+        watchGrp.detectChanges(null, () => changes++);
+
+        expect(child1a.fieldCost).toBe(1);
+        expect(watchGrp.totalFieldCost).toBe(1);
+        expect(changes).toBe(1);
+      });
+
+      it('should be able to move a group with existing watches', () => {
+        var changes = 0,
+            context = {a:'something'};
+
+        setup();
+        child1a = watchGrp.newGroup(context);
+        child1a.watchExpression(parse('a'), logValue('1a'));
+
+        watchGrp.addGroup(child1a);
+        child1a.remove();
+
+        context.a = 'another';
+        watchGrp.detectChanges(null, () => changes++);
+
+        expect(child1a.fieldCost).toBe(1);
+        expect(watchGrp.totalFieldCost).toBe(0);
+        expect(changes).toBe(0);
+
+        child2 = watchGrp.newGroup({});
+        watchGrp.addGroup(child2);
+        child2.addGroup(child1a);
+
+        context.a = 'another2';
+        watchGrp.detectChanges(null, () => changes++);
+
+        expect(child1a.fieldCost).toBe(1);
+        expect(watchGrp.totalFieldCost).toBe(1);
+        expect(changes).toBe(1);
+      });
 
       it('should set field cost to expected value', function() {
         setupChildGroups();
@@ -802,7 +894,7 @@ describe('WatchGroup', function() {
       it('should not call reaction function on removed group', function() {
         setup({ 'name': 'misko' });
 
-        var child = watchGrp.newGroup(context);
+        var child = createAndAddGroup(watchGrp, context);
         watchGrp.watchExpression(parse('name'), function(v, p) {
           logger.log(`root ${v}`);
           if (v === 'destroy') {
@@ -831,7 +923,8 @@ describe('WatchGroup', function() {
         watchGrp.watchExpression(parse('a'), function(v, p) {
           logger.log(v);
         });
-        watchGrp.newGroup(childContext).watchExpression(parse('b'), logCurrentValue);
+
+        createAndAddGroup(watchGrp, childContext).watchExpression(parse('b'), logCurrentValue);
 
         watchGrp.detectChanges();
         expect(logger.toArray()).toEqual(['OK', 'OK']);
@@ -846,3 +939,10 @@ describe('WatchGroup', function() {
     });
   });
 });
+
+function createAndAddGroup(parentGroup, context){
+  var childGroup = parentGroup.newGroup(context);
+  parentGroup.addGroup(childGroup);
+  return childGroup;
+}
+