From c28283f985f72865fbc88aaf989d94434e5b9b56 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 24 Mar 2020 11:24:59 -0700 Subject: [PATCH 1/3] Inlined DevTools event emitter DevTools previously used the NPM events package for dispatching events. This package has an unfortunate flaw though- if a listener throws during event dispatch, no subsequent listeners are called. I've replaced that event dispatcher with my own implementation that ensures all listeners are called before it re-throws an error. This commit replaces that event emitter with a custom implementation that calls all listeners before re-throwing an error. --- packages/react-devtools-shared/package.json | 1 - .../src/backend/agent.js | 2 +- packages/react-devtools-shared/src/bridge.js | 2 +- .../src/devtools/ProfilerStore.js | 2 +- .../src/devtools/store.js | 2 +- packages/react-devtools-shared/src/events.js | 67 +++++++++++++++++++ scripts/flow/react-devtools.js | 17 +---- 7 files changed, 72 insertions(+), 21 deletions(-) create mode 100644 packages/react-devtools-shared/src/events.js diff --git a/packages/react-devtools-shared/package.json b/packages/react-devtools-shared/package.json index 985a51c21131f..52e9523914eba 100644 --- a/packages/react-devtools-shared/package.json +++ b/packages/react-devtools-shared/package.json @@ -11,7 +11,6 @@ "@reach/menu-button": "^0.1.17", "@reach/tooltip": "^0.2.2", "clipboard-js": "^0.3.6", - "events": "^3.0.0", "local-storage-fallback": "^4.1.1", "lodash.throttle": "^4.1.1", "memoize-one": "^3.1.1", diff --git a/packages/react-devtools-shared/src/backend/agent.js b/packages/react-devtools-shared/src/backend/agent.js index b9a2b7ab0af23..f457f4319922d 100644 --- a/packages/react-devtools-shared/src/backend/agent.js +++ b/packages/react-devtools-shared/src/backend/agent.js @@ -7,7 +7,7 @@ * @flow */ -import EventEmitter from 'events'; +import EventEmitter from '../events'; import throttle from 'lodash.throttle'; import { SESSION_STORAGE_LAST_SELECTION_KEY, diff --git a/packages/react-devtools-shared/src/bridge.js b/packages/react-devtools-shared/src/bridge.js index 7bdc37971ee37..c3827f00e2d47 100644 --- a/packages/react-devtools-shared/src/bridge.js +++ b/packages/react-devtools-shared/src/bridge.js @@ -7,7 +7,7 @@ * @flow */ -import EventEmitter from 'events'; +import EventEmitter from './events'; import type {ComponentFilter, Wall} from './types'; import type { diff --git a/packages/react-devtools-shared/src/devtools/ProfilerStore.js b/packages/react-devtools-shared/src/devtools/ProfilerStore.js index 76f58823bef3a..12299a9219d33 100644 --- a/packages/react-devtools-shared/src/devtools/ProfilerStore.js +++ b/packages/react-devtools-shared/src/devtools/ProfilerStore.js @@ -7,7 +7,7 @@ * @flow */ -import EventEmitter from 'events'; +import EventEmitter from '../events'; import {prepareProfilingDataFrontendFromBackendAndStore} from './views/Profiler/utils'; import ProfilingCache from './ProfilingCache'; import Store from './store'; diff --git a/packages/react-devtools-shared/src/devtools/store.js b/packages/react-devtools-shared/src/devtools/store.js index ad46873c35e91..1e246cab0b43e 100644 --- a/packages/react-devtools-shared/src/devtools/store.js +++ b/packages/react-devtools-shared/src/devtools/store.js @@ -7,7 +7,7 @@ * @flow */ -import EventEmitter from 'events'; +import EventEmitter from '../events'; import {inspect} from 'util'; import { TREE_OPERATION_ADD, diff --git a/packages/react-devtools-shared/src/events.js b/packages/react-devtools-shared/src/events.js new file mode 100644 index 0000000000000..e5c13eac38396 --- /dev/null +++ b/packages/react-devtools-shared/src/events.js @@ -0,0 +1,67 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ + +export default class EventEmitter { + listenersMap: Map> = new Map(); + + addListener>( + event: Event, + listener: (...$ElementType) => any, + ): void { + let listeners = this.listenersMap.get(event); + if (listeners === undefined) { + listeners = new Set(); + + this.listenersMap.set(event, listeners); + } + + listeners.add(listener); + } + + emit>( + event: Event, + ...args: $ElementType + ): void { + const listeners = this.listenersMap.get(event); + if (listeners !== undefined) { + let didThrow = false; + let caughtError = null; + + listeners.forEach(listener => { + try { + listener.apply(null, args); + } catch (error) { + if (caughtError === null) { + didThrow = true; + caughtError = error; + } + } + }); + + if (didThrow) { + throw caughtError; + } + } + } + + removeAllListeners(event?: $Keys): void { + if (event != null) { + this.listenersMap.delete(event); + } else { + this.listenersMap.clear(); + } + } + + removeListener(event: $Keys, listener: Function): void { + const listeners = this.listenersMap.get(event); + if (listeners !== undefined) { + listeners.delete(listener); + } + } +} diff --git a/scripts/flow/react-devtools.js b/scripts/flow/react-devtools.js index 26c800da52823..bab0373511474 100644 --- a/scripts/flow/react-devtools.js +++ b/scripts/flow/react-devtools.js @@ -7,19 +7,4 @@ * @flow */ -declare module 'events' { - declare class EventEmitter { - addListener>( - event: Event, - listener: (...$ElementType) => any, - ): void; - emit: >( - event: Event, - ...$ElementType - ) => void; - removeListener(event: $Keys, listener: Function): void; - removeAllListeners(event?: $Keys): void; - } - - declare export default typeof EventEmitter; -} +// No types From cffef3f58e1c5aeddf81c56c4e351f319889f778 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 24 Mar 2020 11:36:01 -0700 Subject: [PATCH 2/3] Added some tests for event emitter --- .../src/__tests__/events-test.js | 96 +++++++++++++++++++ 1 file changed, 96 insertions(+) create mode 100644 packages/react-devtools-shared/src/__tests__/events-test.js diff --git a/packages/react-devtools-shared/src/__tests__/events-test.js b/packages/react-devtools-shared/src/__tests__/events-test.js new file mode 100644 index 0000000000000..be956c05be931 --- /dev/null +++ b/packages/react-devtools-shared/src/__tests__/events-test.js @@ -0,0 +1,96 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ + +describe('events', () => { + let dispatcher; + + beforeEach(() => { + const EventEmitter = require('../events').default; + + dispatcher = new EventEmitter(); + }); + + it('can dispatch an event with no listeners', () => { + dispatcher.emit('event', 123); + }); + + it('handles a listener being attached multiple times', () => { + const callback = jest.fn(); + + dispatcher.addListener('event', callback); + dispatcher.addListener('event', callback); + }); + + it('notifies all attached listeners of events', () => { + const callback1 = jest.fn(); + const callback2 = jest.fn(); + const callback3 = jest.fn(); + + dispatcher.addListener('event', callback1); + dispatcher.addListener('event', callback2); + dispatcher.addListener('other-event', callback3); + dispatcher.emit('event', 123); + + expect(callback1).toHaveBeenCalledTimes(1); + expect(callback1).toHaveBeenCalledWith(123); + expect(callback2).toHaveBeenCalledTimes(1); + expect(callback2).toHaveBeenCalledWith(123); + expect(callback3).not.toHaveBeenCalled(); + }); + + it('calls later listeners before re-throwing if an earlier one throws', () => { + const callbackThatThrows = jest.fn(() => { + throw Error('expected'); + }); + const callback = jest.fn(); + + dispatcher.addListener('event', callbackThatThrows); + dispatcher.addListener('event', callback); + + expect(() => { + dispatcher.emit('event', 123); + }).toThrow('expected'); + + expect(callbackThatThrows).toHaveBeenCalledTimes(1); + expect(callbackThatThrows).toHaveBeenCalledWith(123); + expect(callback).toHaveBeenCalledTimes(1); + expect(callback).toHaveBeenCalledWith(123); + }); + + it('removes attached listeners', () => { + const callback1 = jest.fn(); + const callback2 = jest.fn(); + + dispatcher.addListener('event', callback1); + dispatcher.addListener('other-event', callback2); + dispatcher.removeListener('event', callback1); + dispatcher.emit('event', 123); + expect(callback1).not.toHaveBeenCalled(); + dispatcher.emit('other-event', 123); + expect(callback2).toHaveBeenCalledTimes(1); + expect(callback2).toHaveBeenCalledWith(123); + }); + + it('removes all listeners', () => { + const callback1 = jest.fn(); + const callback2 = jest.fn(); + const callback3 = jest.fn(); + + dispatcher.addListener('event', callback1); + dispatcher.addListener('event', callback2); + dispatcher.addListener('other-event', callback3); + dispatcher.removeAllListeners(); + dispatcher.emit('event', 123); + dispatcher.emit('other-event', 123); + + expect(callback1).not.toHaveBeenCalled(); + expect(callback2).not.toHaveBeenCalled(); + expect(callback3).not.toHaveBeenCalled(); + }); +}); From 87076bd8a51ed830b66ce647bbaa829bac023eb7 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Wed, 25 Mar 2020 09:54:40 -0700 Subject: [PATCH 3/3] DevTools event emitter: Added more tests Clone array before calling listeners in case listeners are added/removed during dispatch. --- .../src/__tests__/events-test.js | 30 ++++++++++ packages/react-devtools-shared/src/events.js | 58 +++++++++++-------- 2 files changed, 63 insertions(+), 25 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/events-test.js b/packages/react-devtools-shared/src/__tests__/events-test.js index be956c05be931..ef1f864deddc4 100644 --- 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 index e5c13eac38396..aacffb3845796 100644 --- a/packages/react-devtools-shared/src/events.js +++ b/packages/react-devtools-shared/src/events.js @@ -8,7 +8,7 @@ */ export default class EventEmitter { - listenersMap: Map> = new Map(); + listenersMap: Map> = new Map(); addListener>( event: Event, @@ -16,12 +16,13 @@ export default class EventEmitter { ): 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>( @@ -30,38 +31,45 @@ export default class EventEmitter { ): 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): void { - if (event != null) { - this.listenersMap.delete(event); - } else { - this.listenersMap.clear(); - } + removeAllListeners(): void { + this.listenersMap.clear(); } removeListener(event: $Keys, 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); + } } } }