From 901e8219aee348aa709d090068c9c8e7e4253482 Mon Sep 17 00:00:00 2001 From: Rob Eisenberg Date: Tue, 27 May 2014 14:04:59 -0400 Subject: [PATCH] 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 --- src/watch_group.js | 65 ++++++++++++--------- test/dirtychecking.spec.js | 21 ++++--- test/observer.spec.js | 9 ++- test/watchgroup.spec.js | 112 +++++++++++++++++++++++++++++++++++-- 4 files changed, 166 insertions(+), 41 deletions(-) diff --git a/src/watch_group.js b/src/watch_group.js index 62b8489..39d6eec 100644 --- a/src/watch_group.js +++ b/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,34 +175,52 @@ 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; @@ -214,16 +228,13 @@ export class WatchGroup { 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; diff --git a/test/dirtychecking.spec.js b/test/dirtychecking.spec.js index 615b1e9..3dea8c5 100644 --- a/test/dirtychecking.spec.js +++ b/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; +} + diff --git a/test/observer.spec.js b/test/observer.spec.js index c73349c..8ef0e8b 100644 --- a/test/observer.spec.js +++ b/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; +} + diff --git a/test/watchgroup.spec.js b/test/watchgroup.spec.js index 9495f2b..655b19d 100644 --- a/test/watchgroup.spec.js +++ b/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; +} +