fixup Model::watch to call all teardowns first

src/common/Model.js

@@ -49,6 +49,7 @@ var Model = Class.extend(EmitterMixin, ListenerMixin, {
 			typeof val === 'object' ||
 			val !== this._props[name]
 		) {
+			this.trigger('before:change:' + name, val);
 			this._props[name] = val;
 			this.trigger('change:' + name, val);
 		}
@@ -56,6 +57,7 @@ var Model = Class.extend(EmitterMixin, ListenerMixin, {
 
 	unset: function(name) {
 		if (this.has(name)) {
+			this.trigger('before:change:' + name, undefined);
 			delete this._props[name];
 			this.trigger('change:' + name, undefined);
 		}
@@ -100,61 +102,59 @@ var Model = Class.extend(EmitterMixin, ListenerMixin, {
 
 	_watchDeps: function(depList, startFunc, stopFunc) {
 		var _this = this;
-		var len = depList.length;
+		var depCnt = depList.length;
 		var satisfyCnt = 0;
 		var values = {}; // what's passed as the `deps` arguments
-		var watchMap = {};
-		var revisionId = 0;
+		var bindTuples = []; // array of [ eventName, handlerFunc ] arrays
+		var isCallingStop = false;
 
-		function reportUpdate(depName, val, isOptional) {
-			var thisRevisionId = ++revisionId;
-
-			// consider optional deps to always have a value
-			if (isOptional && val === undefined) {
-				val = null;
+		function onBeforeDepChange(depName, val, isOptional) {
+			if (satisfyCnt === depCnt) { // all deps previously satisfied?
+				isCallingStop = true;
+				stopFunc();
+				isCallingStop = false;
 			}
+		}
 
-			if (val !== undefined) { // set
+		function onDepChange(depName, val, isOptional) {
 
-				if (!(depName in values)) { // not previously set
-					values[depName] = val;
-					satisfyCnt++;
+			if (val === undefined) { // unsetting a value?
 
-					if (satisfyCnt === len) { // finally now satisfied
-						startFunc(values);
-					}
+				// required dependency that was previously set?
+				if (!isOptional && values[depName] !== undefined) {
+					satisfyCnt--;
 				}
-				// was previously set, so "restart" (call stop+start function)
-				else {
-					if (satisfyCnt === len) { // was satisfied
-						stopFunc();
-						values[depName] = val;
-
-						// if stopFunc changed the value again, don't fire the start function
-						// because it's assumed that will happen soon anyway.
-						if (thisRevisionId === revisionId) {
-							startFunc(values);
-						}
-					}
-					else {
-						values[depName] = val;
-					}
+
+				delete values[depName];
+			}
+			else { // setting a value?
+
+				// required dependency that was previously unset?
+				if (!isOptional && values[depName] === undefined) {
+					satisfyCnt++;
 				}
+
+				values[depName] = val;
 			}
-			else { // unset
 
-				if (depName in values) { // previously set
-					delete values[depName];
-					satisfyCnt--;
+			// now finally satisfied or satisfied all along?
+			if (satisfyCnt === depCnt) {
 
-					if (satisfyCnt === len - 1) { // was previously satisfied
-						stopFunc();
-					}
+				// if the stopFunc initiated another value change, ignore it.
+				// it will be processed by another change event anyway.
+				if (!isCallingStop) {
+					startFunc(values);
 				}
-				// else, not previously set. who cares.
 			}
 		}
 
+		// intercept for .on() that remembers handlers
+		function bind(eventName, handler) {
+			_this.on(eventName, handler);
+			bindTuples.push([ eventName, handler ]);
+		}
+
+		// listen to dependency changes
 		depList.forEach(function(depName) {
 			var isOptional = false;
 
@@ -163,40 +163,48 @@ var Model = Class.extend(EmitterMixin, ListenerMixin, {
 				isOptional = true;
 			}
 
-			var onChange = function(val) {
-				reportUpdate(depName, val, isOptional);
-			};
+			bind('before:change:' + depName, function(val) {
+				onBeforeDepChange(depName, val, isOptional);
+			});
 
-			_this.on('change:' + depName, onChange);
-			watchMap[depName] = onChange;
+			bind('change:' + depName, function(val) {
+				onDepChange(depName, val, isOptional);
+			});
 		});
 
-		if (!len) { // no deps, so resolve immediately
-			startFunc(values);
-		}
-		else {
-			depList.forEach(function(depName) {
-				var isOptional = false;
+		// process current dependency values
+		depList.forEach(function(depName) {
+			var isOptional = false;
 
-				if (depName.charAt(0) === '?') { // TODO: more DRY
-					depName = depName.substring(1);
-					isOptional = true;
-				}
+			if (depName.charAt(0) === '?') { // TODO: more DRY
+				depName = depName.substring(1);
+				isOptional = true;
+			}
 
-				if (isOptional || _this.has(depName)) {
-					reportUpdate(depName, _this.get(depName), isOptional);
-				}
-			});
+			if (_this.has(depName)) {
+				values[depName] = _this.get(depName);
+				satisfyCnt++;
+			}
+			else if (isOptional) {
+				satisfyCnt++;
+			}
+		});
+
+		// initially satisfied
+		if (satisfyCnt === depCnt) {
+			startFunc(values);
 		}
 
 		return function() { // teardown
-			var depName;
 
-			for (depName in watchMap) {
-				_this.off('change:' + depName, watchMap[depName]);
+			// remove all handlers
+			for (var i = 0; i < bindTuples.length; i++) {
+				_this.off(bindTuples[i][0], bindTuples[i][1]);
 			}
+			bindTuples = null;
 
-			if (satisfyCnt === len) { // was satisfied
+			// was satisfied, so call stopFunc
+			if (satisfyCnt === depCnt) {
 				stopFunc();
 			}
 		};

tests/automated-better/Model.js

@@ -131,6 +131,39 @@ describe('Model', function() {
 				});
 			});
 
+			describe('when resetting a dependency', function() {
+
+				it('calls all shutdown funcs before all startup funcs', function() {
+					var m = new Model();
+					var ops = [];
+
+					m.set('var1', 5);
+
+					m.watch('doingSomething1', [ 'var1' ], function() {
+						ops.push('start1');
+					}, function() {
+						ops.push('stop1');
+					});
+
+					m.watch('doingSomething2', [ 'var1' ], function() {
+						ops.push('start2');
+					}, function() {
+						ops.push('stop2');
+					});
+
+					expect(ops).toEqual([
+						'start1', 'start2'
+					]);
+
+					m.set('var1', 6);
+					expect(ops).toEqual([
+						'start1', 'start2',
+						'stop1', 'stop2',
+						'start1', 'start2'
+					]);
+				});
+			});
+
 			describe('with an optional value', function() {
 
 				it('resolves immediately', function() {
@@ -160,12 +193,12 @@ describe('Model', function() {
 					m.set('optvar', 5);
 
 					m.watch('myid', [ '?optvar' ], funcs.start, funcs.stop);
+					expect(stopSpy).not.toHaveBeenCalled();
 					expect(startSpy).toHaveBeenCalledTimes(1);
-					expect(stopSpy).not.toHaveBeenCalledTimes(1);
 
 					m.set('optvar', 6);
+					expect(stopSpy).toHaveBeenCalledTimes(1);
 					expect(startSpy).toHaveBeenCalledTimes(2);
-					expect(stopSpy).not.toHaveBeenCalledTimes(2);
 				});
 
 				it('calls stop/start when value unset', function() {
@@ -180,12 +213,12 @@ describe('Model', function() {
 					m.set('optvar', 5);
 
 					m.watch('myid', [ '?optvar' ], funcs.start, funcs.stop);
+					expect(stopSpy).not.toHaveBeenCalled();
 					expect(startSpy).toHaveBeenCalledTimes(1);
-					expect(stopSpy).not.toHaveBeenCalledTimes(1);
 
 					m.unset('optvar');
+					expect(stopSpy).toHaveBeenCalledTimes(1);
 					expect(startSpy).toHaveBeenCalledTimes(2);
-					expect(stopSpy).not.toHaveBeenCalledTimes(2);
 				});
 			});
 		});