phetsims / axon

Axon provides powerful and concise models for interactive simulations, based on observable Properties and related patterns.
MIT License
11 stars 8 forks source link

TinyEmitter allows addListener when disposed #310

Closed pixelzoom closed 4 years ago

pixelzoom commented 4 years ago

TinyEmitter.addListener (and therefore Emitter.addListener and Property.link) happily allows you to add a listener after it has been disposed. Not until you call emit will you discover your error.

Example:

> var emitter = new phet.axon.Emitter()
undefined
> emitter.addListener( () => {} )
undefined
> emitter.emit()
undefined
> emitter.dispose()
undefined
> emitter.addListener( () => {} )
undefined
> emitter.emit()
assert.js:21 Assertion failed: should not be called if disposed
assert.js:22 Uncaught Error: Assertion failed: should not be called if disposed
    at window.assertions.assertFunction (assert.js:22)
    at TinyEmitter.emit (TinyEmitter.js:54)
    at Emitter.js:33
    at Emitter.execute (Action.js:225)
    at Emitter.emit (Emitter.js:58)
    ...

Shouldn't this be more robust, and detect the programming error at the addListener site, rather than waiting for emit?

Look like this has been the case since Emitter was first created, so assigning to the original author @samreid.

samreid commented 4 years ago

I made this change, added a unit test and tested phet-brand aqua and everything came out clear. I'll commit momentarily.

samreid commented 4 years ago

Committed. @pixelzoom can you please review at your convenience? Should this block publication until the review is complete?

pixelzoom commented 4 years ago

Looks good. Closing.