Skip to content

Commit

Permalink
Adds custom errors for common use cases of using an instance after be…
Browse files Browse the repository at this point in the history
…ing disposed - Fixes #150
  • Loading branch information
mairatma committed Aug 12, 2016
1 parent b0b8798 commit 0527648
Show file tree
Hide file tree
Showing 2 changed files with 83 additions and 26 deletions.
36 changes: 29 additions & 7 deletions packages/metal-state/src/State.js
Original file line number Diff line number Diff line change
Expand Up @@ -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_) : [];
}

/**
Expand All @@ -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;
}
}

/**
Expand All @@ -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.
Expand All @@ -428,7 +430,9 @@ class State extends EventEmitter {
* @return {boolean}
*/
hasStateKey(key) {
return !!this.stateInfo_[key];
if (!this.warnIfDisposed_(key)) {
return !!this.stateInfo_[key];
}
}

/**
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
}
}

/**
Expand Down
73 changes: 54 additions & 19 deletions packages/metal-state/test/State.js
Original file line number Diff line number Diff line change
Expand Up @@ -412,7 +412,7 @@ describe('State', function() {
}
}
};

new Test({
key1: undefined
});
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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() {
Expand Down

0 comments on commit 0527648

Please sign in to comment.