Skip to content

Commit

Permalink
Inlined DevTools event emitter impl (#18378)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Brian Vaughn authored Mar 25, 2020
1 parent 6498f62 commit bd57819
Show file tree
Hide file tree
Showing 8 changed files with 206 additions and 21 deletions.
1 change: 0 additions & 1 deletion packages/react-devtools-shared/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
126 changes: 126 additions & 0 deletions packages/react-devtools-shared/src/__tests__/events-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
/**
* 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);

dispatcher.emit('event', 123);
expect(callback).toHaveBeenCalledTimes(1);
expect(callback).toHaveBeenCalledWith(123);
});

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();
});

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);
});
});
2 changes: 1 addition & 1 deletion packages/react-devtools-shared/src/backend/agent.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
* @flow
*/

import EventEmitter from 'events';
import EventEmitter from '../events';
import throttle from 'lodash.throttle';
import {
SESSION_STORAGE_LAST_SELECTION_KEY,
Expand Down
2 changes: 1 addition & 1 deletion packages/react-devtools-shared/src/bridge.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
* @flow
*/

import EventEmitter from 'events';
import EventEmitter from './events';

import type {ComponentFilter, Wall} from './types';
import type {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down
2 changes: 1 addition & 1 deletion packages/react-devtools-shared/src/devtools/store.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
* @flow
*/

import EventEmitter from 'events';
import EventEmitter from '../events';
import {inspect} from 'util';
import {
TREE_OPERATION_ADD,
Expand Down
75 changes: 75 additions & 0 deletions packages/react-devtools-shared/src/events.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
/**
* 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<Events: Object> {
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) {
this.listenersMap.set(event, [listener]);
} else {
const index = listeners.indexOf(listener);
if (index < 0) {
listeners.push(listener);
}
}
}

emit<Event: $Keys<Events>>(
event: Event,
...args: $ElementType<Events, Event>
): void {
const listeners = this.listenersMap.get(event);
if (listeners !== undefined) {
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;

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;
}
}
}
}

removeAllListeners(): void {
this.listenersMap.clear();
}

removeListener(event: $Keys<Events>, listener: Function): void {
const listeners = this.listenersMap.get(event);
if (listeners !== undefined) {
const index = listeners.indexOf(listener);
if (index >= 0) {
listeners.splice(index, 1);
}
}
}
}
17 changes: 1 addition & 16 deletions scripts/flow/react-devtools.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,4 @@
* @flow
*/

declare module 'events' {
declare class EventEmitter<Events: Object> {
addListener<Event: $Keys<Events>>(
event: Event,
listener: (...$ElementType<Events, Event>) => any,
): void;
emit: <Event: $Keys<Events>>(
event: Event,
...$ElementType<Events, Event>
) => void;
removeListener(event: $Keys<Events>, listener: Function): void;
removeAllListeners(event?: $Keys<Events>): void;
}

declare export default typeof EventEmitter;
}
// No types

0 comments on commit bd57819

Please sign in to comment.