Merge pull request #2224 from kwonoj/feat-fromeventpattern

Align fromEventPattern behavior to RxJS v4

spec/observables/fromEventPattern-spec.ts

@@ -1,5 +1,7 @@
 import {expect} from 'chai';
+import * as sinon from 'sinon';
 import * as Rx from '../../dist/cjs/Rx';
+import {noop} from '../../dist/cjs/util/noop';
 
 declare const rxTestScheduler: Rx.TestScheduler;
 declare const {hot, asDiagram, expectObservable, expectSubscriptions};
@@ -16,58 +18,56 @@ describe('Observable.fromEventPattern', () => {
         .concat(Observable.never())
         .subscribe(h);
     }
-    const e1 = Observable.fromEventPattern(addHandler, () => void 0);
+    const e1 = Observable.fromEventPattern(addHandler);
     const expected = '-----x-x---';
     expectObservable(e1).toBe(expected, {x: 'ev'});
   });
 
   it('should call addHandler on subscription', () => {
-    let addHandlerCalledWith;
-    const addHandler = (h: any) => {
-      addHandlerCalledWith = h;
-    };
+    const addHandler = sinon.spy();
+    Observable.fromEventPattern(addHandler, noop).subscribe(noop);
 
-    const removeHandler = () => {
-      //noop
-    };
+    const call = addHandler.getCall(0);
+    expect(addHandler).calledOnce;
+    expect(call.args[0]).to.be.a('function');
+  });
 
-    Observable.fromEventPattern(addHandler, removeHandler)
-      .subscribe(() => {
-        //noop
-      });
+  it('should call removeHandler on unsubscription', () => {
+    const removeHandler = sinon.spy();
+
+    Observable.fromEventPattern(noop, removeHandler).subscribe(noop).unsubscribe();
 
-    expect(addHandlerCalledWith).to.be.a('function');
+    const call = removeHandler.getCall(0);
+    expect(removeHandler).calledOnce;
+    expect(call.args[0]).to.be.a('function');
   });
 
-  it('should call removeHandler on unsubscription', () => {
-    let removeHandlerCalledWith;
-    const addHandler = () => {
-      //noop
-     };
-    const removeHandler = (h: any) => {
-      removeHandlerCalledWith = h;
-    };
+  it('should work without optional removeHandler', () => {
+    const addHandler: (h: Function) => any = sinon.spy();
+    Observable.fromEventPattern(addHandler).subscribe(noop);
 
-    const subscription = Observable.fromEventPattern(addHandler, removeHandler)
-      .subscribe(() => {
-        //noop
-      });
+    expect(addHandler).calledOnce;
+  });
 
-    subscription.unsubscribe();
+  it('should deliver return value of addHandler to removeHandler as signal', () => {
+    const expected = { signal: true};
+    const addHandler = () => expected;
+    const removeHandler = sinon.spy();
+    Observable.fromEventPattern(addHandler, removeHandler).subscribe(noop).unsubscribe();
 
-    expect(removeHandlerCalledWith).to.be.a('function');
+    const call = removeHandler.getCall(0);
+    expect(call).calledWith(sinon.match.any, expected);
   });
 
-  it('should send errors in addHandler down the error path', () => {
+  it('should send errors in addHandler down the error path', (done: MochaDone) => {
     Observable.fromEventPattern((h: any) => {
       throw 'bad';
-    }, () => {
-        //noop
-      }).subscribe(() => {
-        //noop
-       }, (err: any) => {
-          expect(err).to.equal('bad');
-        });
+    }, noop).subscribe(
+      () => done(new Error('should not be called')),
+      (err: any) => {
+        expect(err).to.equal('bad');
+        done();
+      }, () => done(new Error('should not be called')));
   });
 
   it('should accept a selector that maps outgoing values', (done: MochaDone) => {

src/observable/FromEventPatternObservable.ts

@@ -1,3 +1,4 @@
+import { isFunction } from '../util/isFunction';
 import { Observable } from '../Observable';
 import { Subscription } from '../Subscription';
 import { Subscriber } from '../Subscriber';
@@ -45,9 +46,10 @@ export class FromEventPatternObservable<T> extends Observable<T> {
    * @param {function(handler: Function): any} addHandler A function that takes
    * a `handler` function as argument and attaches it somehow to the actual
    * source of events.
-   * @param {function(handler: Function): void} removeHandler A function that
+   * @param {function(handler: Function, signal?: any): void} [removeHandler] An optional function that
    * takes a `handler` function as argument and removes it in case it was
-   * previously attached using `addHandler`.
+   * previously attached using `addHandler`. if addHandler returns signal to teardown when remove,
+   * removeHandler function will forward it.
    * @param {function(...args: any): T} [selector] An optional function to
    * post-process results. It takes the arguments from the event handler and
    * should return a single value.
@@ -57,13 +59,13 @@ export class FromEventPatternObservable<T> extends Observable<T> {
    * @owner Observable
    */
   static create<T>(addHandler: (handler: Function) => any,
-                   removeHandler: (handler: Function) => void,
+                   removeHandler?: (handler: Function, signal?: any) => void,
                    selector?: (...args: Array<any>) => T) {
     return new FromEventPatternObservable(addHandler, removeHandler, selector);
   }
 
   constructor(private addHandler: (handler: Function) => any,
-              private removeHandler: (handler: Function) => void,
+              private removeHandler?: (handler: Function, signal?: any) => void,
               private selector?: (...args: Array<any>) => T) {
     super();
   }
@@ -75,10 +77,15 @@ export class FromEventPatternObservable<T> extends Observable<T> {
       this._callSelector(subscriber, args);
     } : function(e: any) { subscriber.next(e); };
 
-    this._callAddHandler(handler, subscriber);
+    const retValue = this._callAddHandler(handler, subscriber);
+
+    if (!isFunction(removeHandler)) {
+      return;
+    }
+
     subscriber.add(new Subscription(() => {
       //TODO: determine whether or not to forward to error handler
-      removeHandler(handler);
+      removeHandler(handler, retValue) ;
     }));
   }
 
@@ -92,9 +99,9 @@ export class FromEventPatternObservable<T> extends Observable<T> {
     }
   }
 
-  private _callAddHandler(handler: (e: any) => void, errorSubscriber: Subscriber<T>): void {
+  private _callAddHandler(handler: (e: any) => void, errorSubscriber: Subscriber<T>): any | null {
     try {
-      this.addHandler(handler);
+      return this.addHandler(handler) || null;
     }
     catch (e) {
       errorSubscriber.error(e);