fix(fromEvent): properly teardown for arraylike targets

- smaller impl
ReactiveX · Sep 15, 2020 · 066de74 · 066de74
1 parent b0c2b4a
commit 066de74
Show file tree

Hide file tree

Showing 2 changed files with 81 additions and 40 deletions.
diff --git a/spec/observables/fromEvent-spec.ts b/spec/observables/fromEvent-spec.ts
@@ -3,6 +3,7 @@ import { expectObservable } from '../helpers/marble-testing';
 import { fromEvent, NEVER, timer } from 'rxjs';
 import { mapTo, take, concat } from 'rxjs/operators';
 import { TestScheduler } from 'rxjs/testing';
+import sinon = require('sinon');
 
 declare const rxTestScheduler: TestScheduler;
 
@@ -392,4 +393,49 @@ describe('fromEvent', () => {
     }).to.not.throw(TypeError);
   });
 
+  it('should handle adding events to an arraylike of targets', () => {
+    const nodeList = {
+      [0]: {
+        addEventListener(...args: any[]) {
+          this._addEventListenerArgs = args;
+        },
+        removeEventListener(...args: any[]) {
+          this._removeEventListenerArgs = args;
+        },
+        _addEventListenerArgs: null as any,
+        _removeEventListenerArgs: null as any,
+      },
+      [1]: {
+        addEventListener(...args: any[]) {
+          this._addEventListenerArgs = args;
+        },
+        removeEventListener(...args: any[]) {
+          this._removeEventListenerArgs = args;
+        },
+        _addEventListenerArgs: null as any,
+        _removeEventListenerArgs: null as any,
+      },
+      length: 2
+    };
+
+    const options = {};
+
+    const subscription = fromEvent(nodeList, 'click', options).subscribe();
+
+    expect(nodeList[0]._addEventListenerArgs[0]).to.equal('click');
+    expect(nodeList[0]._addEventListenerArgs[1]).to.be.a('function');
+    expect(nodeList[0]._addEventListenerArgs[2]).to.equal(options);
+
+    expect(nodeList[1]._addEventListenerArgs[0]).to.equal('click');
+    expect(nodeList[1]._addEventListenerArgs[1]).to.be.a('function');
+    expect(nodeList[1]._addEventListenerArgs[2]).to.equal(options);
+
+    expect(nodeList[0]._removeEventListenerArgs).to.be.null;
+    expect(nodeList[1]._removeEventListenerArgs).to.be.null;
+
+    subscription.unsubscribe();
+
+    expect(nodeList[0]._removeEventListenerArgs).to.deep.equal(nodeList[0]._addEventListenerArgs);
+    expect(nodeList[1]._removeEventListenerArgs).to.deep.equal(nodeList[1]._addEventListenerArgs);
+  });
 });
diff --git a/src/internal/observable/fromEvent.ts b/src/internal/observable/fromEvent.ts
@@ -1,7 +1,10 @@
+/** @prettier */
 import { Observable } from '../Observable';
+import { mergeMap } from '../operators/mergeMap';
+import { isArrayLike } from '../util/isArrayLike';
 import { isFunction } from '../util/isFunction';
-import { Subscriber } from '../Subscriber';
 import { mapOneOrManyArgs } from '../util/mapOneOrManyArgs';
+import { fromArray } from './fromArray';
 
 export interface NodeStyleEventEmitter {
   addListener: (eventName: string | symbol, handler: NodeEventHandler) => this;
@@ -49,7 +52,12 @@ export function fromEvent<T>(target: FromEventTarget<T>, eventName: string): Obs
 export function fromEvent<T>(target: FromEventTarget<T>, eventName: string, resultSelector?: (...args: any[]) => T): Observable<T>;
 export function fromEvent<T>(target: FromEventTarget<T>, eventName: string, options?: EventListenerOptions): Observable<T>;
 /** @deprecated resultSelector no longer supported, pipe to map instead */
-export function fromEvent<T>(target: FromEventTarget<T>, eventName: string, options: EventListenerOptions, resultSelector: (...args: any[]) => T): Observable<T>;
+export function fromEvent<T>(
+  target: FromEventTarget<T>,
+  eventName: string,
+  options: EventListenerOptions,
+  resultSelector: (...args: any[]) => T
+): Observable<T>;
 /* tslint:enable:max-line-length */
 
 /**
@@ -175,58 +183,45 @@ export function fromEvent<T>(
   target: FromEventTarget<T>,
   eventName: string,
   options?: EventListenerOptions | ((...args: any[]) => T),
-  resultSelector?: ((...args: any[]) => T)
+  resultSelector?: (...args: any[]) => T
 ): Observable<T> {
-
   if (isFunction(options)) {
     // DEPRECATED PATH
     resultSelector = options;
     options = undefined;
   }
   if (resultSelector) {
     // DEPRECATED PATH
-    return fromEvent<T>(target, eventName, options as EventListenerOptions | undefined).pipe(
-      mapOneOrManyArgs(resultSelector)
-    );
+    return fromEvent<T>(target, eventName, options as EventListenerOptions | undefined).pipe(mapOneOrManyArgs(resultSelector));
   }
 
-  return new Observable<T>(subscriber => {
-    function handler(e: T) {
-      if (arguments.length > 1) {
-        subscriber.next(Array.prototype.slice.call(arguments) as any);
-      } else {
-        subscriber.next(e);
-      }
+  return new Observable<T>((subscriber) => {
+    const handler = (...args: any[]) => subscriber.next(args.length > 1 ? args : args[0]);
+
+    if (isEventTarget(target)) {
+      target.addEventListener(eventName, handler, options as EventListenerOptions);
+      return () => target.removeEventListener(eventName, handler, options as EventListenerOptions);
     }
-    setupSubscription(target, eventName, handler, subscriber, options as EventListenerOptions);
-  });
-}
 
-function setupSubscription<T>(sourceObj: FromEventTarget<T>, eventName: string,
-                              handler: (...args: any[]) => void, subscriber: Subscriber<T>,
-                              options?: EventListenerOptions) {
-  let unsubscribe: (() => void) | undefined;
-  if (isEventTarget(sourceObj)) {
-    const source = sourceObj;
-    sourceObj.addEventListener(eventName, handler, options);
-    unsubscribe = () => source.removeEventListener(eventName, handler, options);
-  } else if (isJQueryStyleEventEmitter(sourceObj)) {
-    const source = sourceObj;
-    sourceObj.on(eventName, handler);
-    unsubscribe = () => source.off(eventName, handler);
-  } else if (isNodeStyleEventEmitter(sourceObj)) {
-    const source = sourceObj;
-    sourceObj.addListener(eventName, handler as NodeEventHandler);
-    unsubscribe = () => source.removeListener(eventName, handler as NodeEventHandler);
-  } else if (sourceObj && (sourceObj as any).length) {
-    for (let i = 0, len = (sourceObj as any).length; i < len; i++) {
-      setupSubscription((sourceObj as any)[i], eventName, handler, subscriber, options);
+    if (isJQueryStyleEventEmitter(target)) {
+      target.on(eventName, handler);
+      return () => target.off(eventName, handler);
+    }
+
+    if (isNodeStyleEventEmitter(target)) {
+      target.addListener(eventName, handler);
+      return () => target.removeListener(eventName, handler);
     }
-  } else {
-    throw new TypeError('Invalid event target');
-  }
 
-  subscriber.add(unsubscribe);
+    if (isArrayLike(target)) {
+      return (mergeMap((target: any) => fromEvent(target, eventName, options as any))(fromArray(target)) as Observable<T>).subscribe(
+        subscriber
+      );
+    }
+
+    subscriber.error(new TypeError('Invalid event target'));
+    return;
+  });
 }
 
 function isNodeStyleEventEmitter(sourceObj: any): sourceObj is NodeStyleEventEmitter {