DevTools event emitter: Added more tests

Clone array before calling listeners in case listeners are added/removed during dispatch.
facebook · Mar 25, 2020 · 87076bd · 87076bd
1 parent cffef3f
commit 87076bd
Show file tree

Hide file tree

Showing 2 changed files with 63 additions and 25 deletions.
diff --git a/packages/react-devtools-shared/src/__tests__/events-test.js b/packages/react-devtools-shared/src/__tests__/events-test.js
@@ -25,6 +25,10 @@ describe('events', () => {
 
     dispatcher.addListener('event', callback);
     dispatcher.addListener('event', callback);
+
+    dispatcher.emit('event', 123);
+    expect(callback).toHaveBeenCalledTimes(1);
+    expect(callback).toHaveBeenCalledWith(123);
   });
 
   it('notifies all attached listeners of events', () => {
@@ -93,4 +97,30 @@ describe('events', () => {
     expect(callback2).not.toHaveBeenCalled();
     expect(callback3).not.toHaveBeenCalled();
   });
+
+  it('should call the initial listeners even if others are added or removed during a dispatch', () => {
+    const callback1 = jest.fn(() => {
+      dispatcher.removeListener('event', callback2);
+      dispatcher.addListener('event', callback3);
+    });
+    const callback2 = jest.fn();
+    const callback3 = jest.fn();
+
+    dispatcher.addListener('event', callback1);
+    dispatcher.addListener('event', callback2);
+
+    dispatcher.emit('event', 123);
+    expect(callback1).toHaveBeenCalledTimes(1);
+    expect(callback1).toHaveBeenCalledWith(123);
+    expect(callback2).toHaveBeenCalledTimes(1);
+    expect(callback2).toHaveBeenCalledWith(123);
+    expect(callback3).not.toHaveBeenCalled();
+
+    dispatcher.emit('event', 456);
+    expect(callback1).toHaveBeenCalledTimes(2);
+    expect(callback1).toHaveBeenCalledWith(456);
+    expect(callback2).toHaveBeenCalledTimes(1);
+    expect(callback3).toHaveBeenCalledTimes(1);
+    expect(callback3).toHaveBeenCalledWith(456);
+  });
 });
diff --git a/packages/react-devtools-shared/src/events.js b/packages/react-devtools-shared/src/events.js
@@ -8,20 +8,21 @@
  */
 
 export default class EventEmitter<Events: Object> {
-  listenersMap: Map<string, Set<Function>> = new Map();
+  listenersMap: Map<string, Array<Function>> = new Map();
 
   addListener<Event: $Keys<Events>>(
     event: Event,
     listener: (...$ElementType<Events, Event>) => any,
   ): void {
     let listeners = this.listenersMap.get(event);
     if (listeners === undefined) {
-      listeners = new Set();
-
-      this.listenersMap.set(event, listeners);
+      this.listenersMap.set(event, [listener]);
+    } else {
+      const index = listeners.indexOf(listener);
+      if (index < 0) {
+        listeners.push(listener);
+      }
     }
-
-    listeners.add(listener);
   }
 
   emit<Event: $Keys<Events>>(
@@ -30,38 +31,45 @@ export default class EventEmitter<Events: Object> {
   ): void {
     const listeners = this.listenersMap.get(event);
     if (listeners !== undefined) {
-      let didThrow = false;
-      let caughtError = null;
+      if (listeners.length === 1) {
+        // No need to clone or try/catch
+        const listener = listeners[0];
+        listener.apply(null, args);
+      } else {
+        let didThrow = false;
+        let caughtError = null;
 
-      listeners.forEach(listener => {
-        try {
-          listener.apply(null, args);
-        } catch (error) {
-          if (caughtError === null) {
-            didThrow = true;
-            caughtError = error;
+        const clonedListeners = Array.from(listeners);
+        for (let i = 0; i < clonedListeners.length; i++) {
+          const listener = clonedListeners[i];
+          try {
+            listener.apply(null, args);
+          } catch (error) {
+            if (caughtError === null) {
+              didThrow = true;
+              caughtError = error;
+            }
           }
         }
-      });
 
-      if (didThrow) {
-        throw caughtError;
+        if (didThrow) {
+          throw caughtError;
+        }
       }
     }
   }
 
-  removeAllListeners(event?: $Keys<Events>): void {
-    if (event != null) {
-      this.listenersMap.delete(event);
-    } else {
-      this.listenersMap.clear();
-    }
+  removeAllListeners(): void {
+    this.listenersMap.clear();
   }
 
   removeListener(event: $Keys<Events>, listener: Function): void {
     const listeners = this.listenersMap.get(event);
     if (listeners !== undefined) {
-      listeners.delete(listener);
+      const index = listeners.indexOf(listener);
+      if (index >= 0) {
+        listeners.splice(index, 1);
+      }
     }
   }
 }