diff --git a/packages/messenger/CHANGELOG.md b/packages/messenger/CHANGELOG.md index 35c312f9893..276135abce2 100644 --- a/packages/messenger/CHANGELOG.md +++ b/packages/messenger/CHANGELOG.md @@ -7,6 +7,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Add `captureException` constructor parameter ([#6605](https://github.com/MetaMask/core/pull/6605)) + - This function will be used to capture any errors thrown from subscribers. + - If this is unset but a parent is provided, `captureException` is inherited from the parent. + +### Changed + +- Stop re-throwing subscriber errors in a `setTimeout` ([#6605](https://github.com/MetaMask/core/pull/6605)) + - Instead errors are captured with `captureException`, or logged to the console. + ## [0.2.0] ### Added diff --git a/packages/messenger/src/Messenger.test.ts b/packages/messenger/src/Messenger.test.ts index fbac739a3ee..5e59910340c 100644 --- a/packages/messenger/src/Messenger.test.ts +++ b/packages/messenger/src/Messenger.test.ts @@ -810,26 +810,89 @@ describe('Messenger', () => { expect(selector.callCount).toBe(4); }); - it('throws subscriber errors in a timeout', () => { - const setTimeoutStub = sinon.stub(globalThis, 'setTimeout'); + it('captures subscriber errors using captureException', () => { + const captureException = jest.fn(); type MessageEvent = { type: 'Fixture:message'; payload: [string] }; const messenger = new Messenger<'Fixture', never, MessageEvent>({ + captureException, namespace: 'Fixture', }); + const exampleError = new Error('Example error'); - const handler = sinon.stub().throws(() => new Error('Example error')); + const handler = sinon.stub().throws(() => exampleError); messenger.subscribe('Fixture:message', handler); expect(() => messenger.publish('Fixture:message', 'hello')).not.toThrow(); - expect(setTimeoutStub.callCount).toBe(1); - const onTimeout = setTimeoutStub.firstCall.args[0]; - expect(() => onTimeout()).toThrow('Example error'); + expect(captureException).toHaveBeenCalledTimes(1); + expect(captureException).toHaveBeenCalledWith(exampleError); + }); + + it('captures subscriber thrown non-errors using captureException', () => { + const captureException = jest.fn(); + type MessageEvent = { type: 'Fixture:message'; payload: [string] }; + const messenger = new Messenger<'Fixture', never, MessageEvent>({ + captureException, + namespace: 'Fixture', + }); + const exampleException = 'Non-error thrown value'; + + const handler = sinon.stub().throws(() => exampleException); + messenger.subscribe('Fixture:message', handler); + + expect(() => messenger.publish('Fixture:message', 'hello')).not.toThrow(); + expect(captureException).toHaveBeenCalledTimes(1); + expect(captureException).toHaveBeenCalledWith( + new Error(exampleException), + ); + }); + + it('captures subscriber errors using inherited captureException', () => { + const captureException = jest.fn(); + type MessageEvent = { type: 'Fixture:message'; payload: [string] }; + const parentMessenger = new Messenger<'Parent', never, MessageEvent>({ + captureException, + namespace: 'Parent', + }); + const messenger = new Messenger< + 'Fixture', + never, + MessageEvent, + typeof parentMessenger + >({ + namespace: 'Fixture', + parent: parentMessenger, + }); + const exampleError = new Error('Example error'); + + const handler = sinon.stub().throws(() => exampleError); + messenger.subscribe('Fixture:message', handler); + + expect(() => messenger.publish('Fixture:message', 'hello')).not.toThrow(); + expect(captureException).toHaveBeenCalledTimes(1); + expect(captureException).toHaveBeenCalledWith(exampleError); + }); + + it('logs subscriber errors to console if no captureException provided', () => { + const consoleError = jest.fn(); + jest.spyOn(console, 'error').mockImplementation(consoleError); + type MessageEvent = { type: 'Fixture:message'; payload: [string] }; + const messenger = new Messenger<'Fixture', never, MessageEvent>({ + namespace: 'Fixture', + }); + const exampleError = new Error('Example error'); + + const handler = sinon.stub().throws(() => exampleError); + messenger.subscribe('Fixture:message', handler); + + expect(() => messenger.publish('Fixture:message', 'hello')).not.toThrow(); + expect(consoleError).toHaveBeenCalledTimes(1); + expect(consoleError).toHaveBeenCalledWith(exampleError); }); it('continues calling subscribers when one throws', () => { - const setTimeoutStub = sinon.stub(globalThis, 'setTimeout'); type MessageEvent = { type: 'Fixture:message'; payload: [string] }; const messenger = new Messenger<'Fixture', never, MessageEvent>({ + captureException: jest.fn(), namespace: 'Fixture', }); @@ -844,9 +907,6 @@ describe('Messenger', () => { expect(handler1.callCount).toBe(1); expect(handler2.calledWithExactly('hello')).toBe(true); expect(handler2.callCount).toBe(1); - expect(setTimeoutStub.callCount).toBe(1); - const onTimeout = setTimeoutStub.firstCall.args[0]; - expect(() => onTimeout()).toThrow('Example error'); }); it('does not call subscriber after unsubscribing', () => { diff --git a/packages/messenger/src/Messenger.ts b/packages/messenger/src/Messenger.ts index 54a84fbaab9..097b063f036 100644 --- a/packages/messenger/src/Messenger.ts +++ b/packages/messenger/src/Messenger.ts @@ -177,6 +177,7 @@ type DelegatedMessenger = Pick< | '_internalRegisterDelegatedActionHandler' | '_internalRegisterDelegatedInitialEventPayload' | '_internalUnregisterDelegatedActionHandler' + | 'captureException' >; type StripNamespace = @@ -254,6 +255,13 @@ export class Messenger< unknown | undefined >(); + /** + * Reports an error to an error monitoring service. + * + * @param error - The error to report. + */ + readonly captureException?: (error: Error) => void; + /** * Construct a messenger. * @@ -261,13 +269,16 @@ export class Messenger< * be delegated to the parent automatically. * * @param args - Constructor arguments + * @param args.captureException - Reports an error to an error monitoring service. * @param args.namespace - The messenger namespace. * @param args.parent - The parent messenger. */ constructor({ + captureException, namespace, parent, }: { + captureException?: (error: Error) => void; namespace: Namespace; parent?: Action['type'] extends MessengerActions['type'] ? Event['type'] extends MessengerEvents['type'] @@ -277,6 +288,7 @@ export class Messenger< }) { this.#namespace = namespace; this.#parent = parent; + this.captureException = captureException ?? this.#parent?.captureException; } /** @@ -529,11 +541,14 @@ export class Messenger< (handler as GenericEventHandler)(...payload); } } catch (error) { - // Throw error after timeout so that it is capured as a console error - // (and by Sentry) without interrupting the event publishing. - setTimeout(() => { - throw error; - }); + // Capture error without interrupting the event publishing. + if (this.captureException) { + this.captureException( + error instanceof Error ? error : new Error(String(error)), + ); + } else { + console.error(error); + } } } }