Merge pull request #1599 from matrix-org/dbkr/reemitter_dont_throw_if_no_error_handler

ReEmitter: Don't throw if no error handler is attached

Author: dbkr

spec/unit/ReEmitter.spec.ts

@@ -23,6 +23,10 @@ class EventSource extends EventEmitter {
     doTheThing() {
         this.emit(EVENTNAME, "foo", "bar");
     }
+
+    doAnError() {
+        this.emit('error');
+    }
 }
 
 class EventTarget extends EventEmitter {
@@ -46,4 +50,23 @@ describe("ReEmitter", function() {
         // also the source object of the event which re-emitter adds
         expect(handler).toHaveBeenCalledWith("foo", "bar", src);
     });
+
+    it("Doesn't throw if no handler for 'error' event", function() {
+        const src = new EventSource();
+        const tgt = new EventTarget();
+
+        const reEmitter = new ReEmitter(tgt);
+        reEmitter.reEmit(src, ['error']);
+
+        // without the workaround in ReEmitter, this would throw
+        src.doAnError();
+
+        const handler = jest.fn();
+        tgt.on('error', handler);
+
+        src.doAnError();
+
+        // Now we've attached an error handler, it should be called
+        expect(handler).toHaveBeenCalled();
+    });
 });

src/ReEmitter.ts

@@ -31,6 +31,17 @@ export class ReEmitter {
             // such as read receipt listeners on the client class which won't have the context
             // of the room.
             const forSource = (...args) => {
+                // EventEmitter special cases 'error' to make the emit function throw if no
+                // handler is attached, which sort of makes sense for making sure that something
+                // handles an error, but for re-emitting, there could be a listener on the original
+                // source object so the test doesn't really work. We *could* try to replicate the
+                // same logic and throw if there is no listener on either the source or the target,
+                // but this behaviour is fairly undesireable for us anyway: the main place we throw
+                // 'error' events is for calls, where error events are usually emitted some time
+                // later by a different part of the code where 'emit' throwing because the app hasn't
+                // added an error handler isn't terribly helpful. (A better fix in retrospect may
+                // have been to just avoid using the event name 'error', but backwards compat...)
+                if (eventName === 'error' && this.target.listenerCount('error') === 0) return;
                 this.target.emit(eventName, ...args, source);
             };
             source.on(eventName, forSource);