Merge pull request #191 from deepstreamIO/bug/#190-unsubscribe

Bug/#190 unsubscribe

src/record/list.js

@@ -172,10 +172,22 @@ List.prototype.subscribe = function() {
 	}
 
 	//Make sure the callback is invoked with an empty array for new records
-	parameters.callback = function( callback ) {
+	var listCallback = function( callback ) {
 		callback( this.getEntries() );
 	}.bind( this, parameters.callback );
 
+	/**
+	* Adding a property onto a function directly is terrible practice,
+	* and we will change this as soon as we have a more seperate approach
+	* of creating lists that doesn't have records default state.
+	*
+	* The reason we are holding a referencing to wrapped array is so that
+	* on unsubscribe it can provide a reference to the actual method the
+	* record is subscribed too.
+	**/
+	parameters.callback.wrappedCallback = listCallback;
+	parameters.callback = listCallback;
+
 	this._record.subscribe( parameters );
 };
 
@@ -193,6 +205,7 @@ List.prototype.unsubscribe = function() {
 		throw new Error( 'path is not supported for List.unsubscribe' );
 	}
 
+	parameters.callback = parameters.callback.wrappedCallback;
 	this._record.unsubscribe( parameters );
 };
 

src/record/record.js

@@ -176,7 +176,7 @@ Record.prototype.set = function( pathOrData, data ) {
  * @returns {void}
  */
 Record.prototype.subscribe = function( path, callback, triggerNow ) {
-	var i, args = this._normalizeArguments( arguments );
+	var args = this._normalizeArguments( arguments );
 
 	if( this._checkDestroyed( 'subscribe' ) ) {
 		return;
@@ -211,11 +211,16 @@ Record.prototype.subscribe = function( path, callback, triggerNow ) {
  * @returns {void}
  */
 Record.prototype.unsubscribe = function( pathOrCallback, callback ) {
+	var args = this._normalizeArguments( arguments );
+
 	if( this._checkDestroyed( 'unsubscribe' ) ) {
 		return;
 	}
-	var event = arguments.length === 2 ? pathOrCallback : ALL_EVENT;
-	this._eventEmitter.off( event, callback );
+	if ( args.path ) {
+		this._eventEmitter.off( args.path, args.callback );
+	} else {
+		this._eventEmitter.off( ALL_EVENT, args.callback );
+	}
 };
 
 /**

test-unit/unit/record/record-subscriptionSpec.js

@@ -6,12 +6,14 @@ var Record = require( '../../../src/record/record.js' ),
 describe( 'supscriptions to local record changes', function(){
 	var record,
 		callback = jasmine.createSpy( 'firstnameCallback' ),
-		generalCallback = jasmine.createSpy( 'general' ),
+		generalCallback = jasmine.createSpy( 'generalCallback' ),
+		anotherGeneralCallback = jasmine.createSpy( 'anotherGeneralCallback' ),
 		connection = new MockConnection();
 
 	it( 'creates the record', function(){
 		record = new Record( 'testRecord', {}, connection, options, new ClientMock() );
 		record.subscribe( generalCallback );
+		record.subscribe( anotherGeneralCallback );
 		expect( generalCallback ).not.toHaveBeenCalled();
 		record._$onMessage({ topic: 'RECORD', action: 'R', data: [ 'testRecord', 0, '{}' ]} );
 		expect( generalCallback ).not.toHaveBeenCalled();
@@ -38,16 +40,36 @@ describe( 'supscriptions to local record changes', function(){
 		expect( record.get() ).toEqual({ firstname: 'Wolfram', lastname: 'Hempel' });
 	});
 
-	it( 'unsubscribes', function(){
+	it( 'unsubscribes path', function(){
 		record.set( 'firstname', 'Egon' );
 		expect( callback ).toHaveBeenCalledWith( 'Egon' );
 		expect( callback.calls.count() ).toEqual( 2 );
+
 		record.unsubscribe( 'firstname', callback );
+
 		record.set( 'firstname', 'Ray' );
-		expect( callback ).toHaveBeenCalledWith( 'Egon' );
 		expect( callback.calls.count() ).toEqual( 2 );
 	});
 
+	it( 'unsubscribes general callback', function(){
+		record.set( 'firstname', 'Egon' );
+		expect( generalCallback ).toHaveBeenCalledWith({ firstname: 'Egon', lastname: 'Hempel' });
+		expect( generalCallback.calls.count() ).toEqual( 5 );
+		expect( anotherGeneralCallback ).toHaveBeenCalledWith({ firstname: 'Egon', lastname: 'Hempel' });
+		expect( anotherGeneralCallback.calls.count() ).toEqual( 5 );
+
+		record.unsubscribe( generalCallback );
+
+		// This line ensures that unsubscribing something that wasnt registered
+		// does nothing. See #190 if your curious
+		record.unsubscribe( function() {} );
+
+		record.set( 'firstname', 'Ray' );
+		expect( generalCallback.calls.count() ).toEqual( 5 );
+		expect( anotherGeneralCallback ).toHaveBeenCalledWith({ firstname: 'Ray', lastname: 'Hempel' });
+		expect( anotherGeneralCallback.calls.count() ).toEqual( 6 );
+	});
+
 	it( 'subscribes to a deep path', function(){
 		record.subscribe( 'addresses[ 1 ].street', callback );
 		record.set( 'addresses[ 1 ].street', 'someStreet' );