From 05276488ef72dc3feff2b0e62d319b8087ac9a0f Mon Sep 17 00:00:00 2001
From: Maira Bello <maira.araujo@liferay.com>
Date: Fri, 12 Aug 2016 16:03:44 -0300
Subject: [PATCH] Adds custom errors for common use cases of using an instance
 after being disposed - Fixes #150

---
 packages/metal-state/src/State.js  | 36 ++++++++++++---
 packages/metal-state/test/State.js | 73 ++++++++++++++++++++++--------
 2 files changed, 83 insertions(+), 26 deletions(-)

diff --git a/packages/metal-state/src/State.js b/packages/metal-state/src/State.js
index b621648f..59b4a424 100644
--- a/packages/metal-state/src/State.js
+++ b/packages/metal-state/src/State.js
@@ -382,10 +382,10 @@ class State extends EventEmitter {
 
 	/**
 	 * Returns an array with all state keys.
-	 * @return {Array.<string>}
+	 * @return {!Array.<string>}
 	 */
 	getStateKeys() {
-		return Object.keys(this.stateInfo_);
+		return this.stateInfo_ ? Object.keys(this.stateInfo_) : [];
 	}
 
 	/**
@@ -396,8 +396,10 @@ class State extends EventEmitter {
 	 * @protected
 	 */
 	getStateKeyValue_(name) {
-		this.initStateKey_(name);
-		return this.stateInfo_[name].value;
+		if (!this.warnIfDisposed_(name)) {
+			this.initStateKey_(name);
+			return this.stateInfo_[name].value;
+		}
 	}
 
 	/**
@@ -411,7 +413,7 @@ class State extends EventEmitter {
 		return info.state === State.KeyStates.INITIALIZED ||
 			this.hasInitialValue_(name);
 	}
-	
+
 	/**
 	 * Checks if an initial value was given to the specified state property.
 	 * @param {string} name The name of the key.
@@ -428,7 +430,9 @@ class State extends EventEmitter {
 	 * @return {boolean}
 	 */
 	hasStateKey(key) {
-		return !!this.stateInfo_[key];
+		if (!this.warnIfDisposed_(key)) {
+			return !!this.stateInfo_[key];
+		}
 	}
 
 	/**
@@ -612,7 +616,9 @@ class State extends EventEmitter {
 	 * @protected
 	 */
 	setStateKeyValue_(name, value) {
-		if (!this.canSetState(name) || !this.validateKeyValue_(name, value)) {
+		if (this.warnIfDisposed_(name) ||
+			!this.canSetState(name) ||
+			!this.validateKeyValue_(name, value)) {
 			return;
 		}
 
@@ -660,6 +666,22 @@ class State extends EventEmitter {
 		return info.state === State.KeyStates.INITIALIZING ||
 			this.callValidator_(name, value);
 	}
+
+	/**
+	 * Warns if this instance has already been disposed.
+	 * @param {string} name Name of the property to be accessed if not disposed.
+	 * @return {boolean} True if disposed, or false otherwise.
+	 * @protected
+	 */
+	warnIfDisposed_(name) {
+		const disposed = this.isDisposed();
+		if (disposed) {
+			console.warn(
+				`Error. Trying to access property "${name}" on disposed instance`
+			);
+		}
+		return disposed;
+	}
 }
 
 /**
diff --git a/packages/metal-state/test/State.js b/packages/metal-state/test/State.js
index cb6893a5..a75ac09d 100644
--- a/packages/metal-state/test/State.js
+++ b/packages/metal-state/test/State.js
@@ -412,7 +412,7 @@ describe('State', function() {
 				}
 			}
 		};
-		
+
 		new Test({
 			key1: undefined
 		});
@@ -713,16 +713,6 @@ describe('State', function() {
 		state.key2 = 21;
 	});
 
-	it('should not throw error when trying to emit scheduled stateChanged after disposed', function(done) {
-		var state = createStateInstance();
-
-		state.key1 = 10;
-		state.dispose();
-		async.nextTick(function() {
-			done();
-		});
-	});
-
 	it('should call callback function from setState asynchronously after the batch event is triggered', function(done) {
 		var state = createStateInstance();
 
@@ -816,14 +806,6 @@ describe('State', function() {
 		assert.strictEqual(0, listener.callCount);
 	});
 
-	it('should not allow getting state data after disposed', function() {
-		var state = createStateInstance();
-		state.dispose();
-		assert.throws(function() {
-			state.getState();
-		});
-	});
-
 	describe('Static STATE', function() {
 		function createTestClass() {
 			class Test extends State {
@@ -1083,6 +1065,59 @@ describe('State', function() {
 			assert.strictEqual(2, listener.callCount);
 		});
 	});
+
+	describe('Dispose', function() {
+		const originalWarnFn = console.warn;
+
+		beforeEach(function() {
+			console.warn = sinon.stub();
+		});
+
+		afterEach(function() {
+			console.warn = originalWarnFn;
+		});
+
+		it('should not throw error when trying to emit scheduled stateChanged after disposed', function(done) {
+			var state = createStateInstance();
+
+			state.key1 = 10;
+			state.dispose();
+			async.nextTick(function() {
+				done();
+			});
+		});
+
+		it('should warn if trying to get state property after disposed', function() {
+			var state = createStateInstance();
+			state.dispose();
+			assert.doesNotThrow(() => state.key1);
+			assert.strictEqual(1, console.warn.callCount);
+		});
+
+		it('should warn if trying to set state property after disposed', function() {
+			var state = createStateInstance();
+			state.dispose();
+			assert.doesNotThrow(() => state.key1 = 'new');
+			assert.strictEqual(1, console.warn.callCount);
+		});
+
+		it('should warn if trying to use "setState" after disposed', function() {
+			var state = createStateInstance();
+			state.dispose();
+			assert.doesNotThrow(() => {
+				state.setState({
+					key1: 'new'
+				});
+			});
+			assert.strictEqual(1, console.warn.callCount);
+		});
+
+		it('should not throw error if trying to use "getState" after disposed', function() {
+			var state = createStateInstance();
+			state.dispose();
+			assert.doesNotThrow(() => state.getState());
+		});
+	});
 });
 
 function createStateInstance() {