From 7dd9a6033d90ab9dbf0e85435afde52263a1d0d8 Mon Sep 17 00:00:00 2001 From: Kurt Preston Date: Tue, 21 Jan 2025 19:27:22 -0600 Subject: [PATCH 1/6] Prefer for..of loops to forEach for perf --- src/strongbus.ts | 50 +++++++++++++++++++++++++++--------------------- 1 file changed, 28 insertions(+), 22 deletions(-) diff --git a/src/strongbus.ts b/src/strongbus.ts index 53f7de8..c7717be 100644 --- a/src/strongbus.ts +++ b/src/strongbus.ts @@ -1,6 +1,6 @@ import {autobind} from 'core-decorators'; -import {CancelablePromise, cancelable, timeout} from 'jaasync'; +import {CancelablePromise, cancelable, timeout, isPromise} from 'jaasync'; import {Scanner} from './scanner'; import {StrongbusLogger} from './strongbusLogger'; @@ -528,19 +528,21 @@ export class Bus implements public get listeners(): ReadonlyMap|Events.WILDCARD, ReadonlySet> { if(!this._cachedGetListersValue) { const listenerCache = new Map(this.ownListeners); - this._delegates.forEach((_, delegate) => { - delegate.listeners.forEach((delegateListeners, event) => { + for(const delegate of this._delegates.keys()) { + for(const [event, delegateListeners] of delegate.listeners) { if(!delegateListeners.size) { - return; + continue; } let listeners = listenerCache.get(event); if(!listeners) { listeners = new Set(); listenerCache.set(event, listeners); } - delegateListeners.forEach(d => (listeners as Set).add(d)); - }); - }); + for(const listener of delegateListeners) { + (listeners as Set).add(listener); + } + } + } this._cachedGetListersValue = listenerCache; } return this._cachedGetListersValue; @@ -550,11 +552,11 @@ export class Bus implements public get ownListeners(): ReadonlyMap|Events.WILDCARD, ReadonlySet>> { if(!this._cachedGetOwnListenersValue) { const ownListenerCache = new Map|Events.WILDCARD, Set>>(); - this.bus.forEach((listeners, event) => { + for(const [event, listeners] of this.bus) { if(listeners.size) { ownListenerCache.set(event, new Set(listeners)); } - }); + } this._cachedGetOwnListenersValue = ownListenerCache; } return this._cachedGetOwnListenersValue; @@ -610,7 +612,11 @@ export class Bus implements } private releaseDelegates(): void { - this._delegates.forEach(subs => over(subs)()); + for(const subs of Object.values(this._delegates)) { + for(const sub of subs()) { + sub(); + } + } this._delegates.clear(); } @@ -675,13 +681,14 @@ export class Bus implements private emitEvent(event: EventKeys|Events.WILDCARD, ...args: any[]): boolean { const handlers = this.bus.get(event); if(handlers && handlers.size) { - handlers.forEach(async fn => { - try { - await fn(...args); - } catch(e) { + for(const fn of handlers) { + const execution = fn(...args); + + // Bubble up errors if fn returns promise + (execution as Promise)?.catch?.((e) => { this.emitLifecycleEvent(Lifecycle.error, {error: e, event}); - } - }); + }); + } return true; } return false; @@ -690,10 +697,9 @@ export class Bus implements private emitLifecycleEvent(event: L, payload: Lifecycle.EventMap[L]): void { const handlers = this.lifecycle.get(event); if(handlers && handlers.size) { - handlers.forEach(async fn => { - try { - await fn(payload); - } catch(e) { + for(const fn of handlers) { + const execution = fn(payload); + (execution as Promise)?.catch?.((e) => { if(event === Lifecycle.error) { const errorPayload = payload as Lifecycle.EventMap['error']; this.options.logger.error('Error thrown in error handler', { @@ -705,8 +711,8 @@ export class Bus implements } else { this.emitLifecycleEvent(Lifecycle.error, {error: e, event}); } - } - }); + }) + } } } From dc738427b2c7d2b43f324c4bf44af71202534162 Mon Sep 17 00:00:00 2001 From: Kurt Preston Date: Tue, 21 Jan 2025 19:31:16 -0600 Subject: [PATCH 2/6] Handle syncronous errors --- src/strongbus.ts | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/src/strongbus.ts b/src/strongbus.ts index c7717be..d0d6cf4 100644 --- a/src/strongbus.ts +++ b/src/strongbus.ts @@ -682,12 +682,16 @@ export class Bus implements const handlers = this.bus.get(event); if(handlers && handlers.size) { for(const fn of handlers) { - const execution = fn(...args); + try { + const execution = fn(...args); - // Bubble up errors if fn returns promise - (execution as Promise)?.catch?.((e) => { + // Bubble up errors if fn returns promise + (execution as Promise)?.catch?.((e) => { + this.emitLifecycleEvent(Lifecycle.error, {error: e, event}); + }); + } catch(e) { this.emitLifecycleEvent(Lifecycle.error, {error: e, event}); - }); + } } return true; } @@ -698,8 +702,22 @@ export class Bus implements const handlers = this.lifecycle.get(event); if(handlers && handlers.size) { for(const fn of handlers) { - const execution = fn(payload); - (execution as Promise)?.catch?.((e) => { + try { + const execution = fn(payload); + (execution as Promise)?.catch?.((e) => { + if(event === Lifecycle.error) { + const errorPayload = payload as Lifecycle.EventMap['error']; + this.options.logger.error('Error thrown in error handler', { + errorHandler: fn.name, + errorHandlerError: e, + originalEvent: errorPayload.event, + eventHandlerError: errorPayload.error + }); + } else { + this.emitLifecycleEvent(Lifecycle.error, {error: e, event}); + } + }) + } catch(e) { if(event === Lifecycle.error) { const errorPayload = payload as Lifecycle.EventMap['error']; this.options.logger.error('Error thrown in error handler', { @@ -711,7 +729,7 @@ export class Bus implements } else { this.emitLifecycleEvent(Lifecycle.error, {error: e, event}); } - }) + } } } } From 3d30e4ecb7b0b2910e3bc887876b49d94b924f9c Mon Sep 17 00:00:00 2001 From: Kurt Preston Date: Tue, 21 Jan 2025 19:32:09 -0600 Subject: [PATCH 3/6] Remove isPromise --- src/strongbus.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/strongbus.ts b/src/strongbus.ts index d0d6cf4..3978117 100644 --- a/src/strongbus.ts +++ b/src/strongbus.ts @@ -1,6 +1,6 @@ import {autobind} from 'core-decorators'; -import {CancelablePromise, cancelable, timeout, isPromise} from 'jaasync'; +import {CancelablePromise, cancelable, timeout} from 'jaasync'; import {Scanner} from './scanner'; import {StrongbusLogger} from './strongbusLogger'; From 66229d1846599df9034a1d7f31ab83bdb6837b49 Mon Sep 17 00:00:00 2001 From: Kurt Preston Date: Wed, 22 Jan 2025 12:14:39 -0600 Subject: [PATCH 4/6] Adding comments --- src/strongbus.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/strongbus.ts b/src/strongbus.ts index 3978117..c08e20e 100644 --- a/src/strongbus.ts +++ b/src/strongbus.ts @@ -685,11 +685,12 @@ export class Bus implements try { const execution = fn(...args); - // Bubble up errors if fn returns promise + // Emit errors if fn returns promise that rejects (execution as Promise)?.catch?.((e) => { this.emitLifecycleEvent(Lifecycle.error, {error: e, event}); }); } catch(e) { + // Emit errors if callback fails synchronously this.emitLifecycleEvent(Lifecycle.error, {error: e, event}); } } @@ -704,10 +705,11 @@ export class Bus implements for(const fn of handlers) { try { const execution = fn(payload); + // Emit errors if fn returns promise that rejects (execution as Promise)?.catch?.((e) => { if(event === Lifecycle.error) { const errorPayload = payload as Lifecycle.EventMap['error']; - this.options.logger.error('Error thrown in error handler', { + this.options.logger.error('Error thrown in async error handler', { errorHandler: fn.name, errorHandlerError: e, originalEvent: errorPayload.event, @@ -718,6 +720,7 @@ export class Bus implements } }) } catch(e) { + // Emit errors if callback fails synchronously if(event === Lifecycle.error) { const errorPayload = payload as Lifecycle.EventMap['error']; this.options.logger.error('Error thrown in error handler', { From cdff57174621c3679d9ce5fa5e45fecf91f09a32 Mon Sep 17 00:00:00 2001 From: Kurt Preston Date: Wed, 22 Jan 2025 12:33:37 -0600 Subject: [PATCH 5/6] Adding spec for erroring on rejected promises --- src/strongbus.ts | 1 + src/strongbus_spec.ts | 16 ++++++++++++++++ 2 files changed, 17 insertions(+) diff --git a/src/strongbus.ts b/src/strongbus.ts index c08e20e..85cb4e8 100644 --- a/src/strongbus.ts +++ b/src/strongbus.ts @@ -705,6 +705,7 @@ export class Bus implements for(const fn of handlers) { try { const execution = fn(payload); + // Emit errors if fn returns promise that rejects (execution as Promise)?.catch?.((e) => { if(event === Lifecycle.error) { diff --git a/src/strongbus_spec.ts b/src/strongbus_spec.ts index 77951e3..cd676d0 100644 --- a/src/strongbus_spec.ts +++ b/src/strongbus_spec.ts @@ -798,6 +798,22 @@ describe('Strongbus.Bus', () => { }); }); + it('raises "error" when the listener returns a promise that rejects', async () => { + const error = new Error('Error in callback'); + bus.on('bar', () => ( + Promise.reject(error) + )); + bus.emit('bar', true); + + // Wait for promises to be processed + await Promise.resolve(); + + expect(onError).toHaveBeenCalledWith({ + error, + event: 'bar' + }); + }); + describe('given bus has delegates', () => { let delegate: DelegateTestBus; let onDelegateWillAddListener: jasmine.Spy; From c9b72d8474400209462b19c84c74c4c78c6da4fa Mon Sep 17 00:00:00 2001 From: Kurt Preston Date: Wed, 22 Jan 2025 12:40:28 -0600 Subject: [PATCH 6/6] More error specs --- src/strongbus_spec.ts | 70 ++++++++++++++++++++++++++++++------------- 1 file changed, 50 insertions(+), 20 deletions(-) diff --git a/src/strongbus_spec.ts b/src/strongbus_spec.ts index cd676d0..176df2d 100644 --- a/src/strongbus_spec.ts +++ b/src/strongbus_spec.ts @@ -786,31 +786,61 @@ describe('Strongbus.Bus', () => { sub2(); }); - it('raises "error" when errors are thrown in the listener', () => { - const error = new Error('Error in callback'); - bus.on('bar', () => { - throw error; + describe('error events', () => { + it('emits "error" when errors are thrown in the listener', () => { + const error = new Error('Error in callback'); + bus.on('bar', () => { + throw error; + }); + bus.emit('bar', true); + expect(onError).toHaveBeenCalledWith({ + error, + event: 'bar' + }); }); - bus.emit('bar', true); - expect(onError).toHaveBeenCalledWith({ - error, - event: 'bar' + + it('emits "error" when the listener returns a promise that rejects', async () => { + const error = new Error('Error in callback'); + bus.on('bar', () => ( + Promise.reject(error) + )); + bus.emit('bar', true); + + // Wait for promises to be processed + await Promise.resolve(); + + expect(onError).toHaveBeenCalledWith({ + error, + event: 'bar' + }); }); - }); - it('raises "error" when the listener returns a promise that rejects', async () => { - const error = new Error('Error in callback'); - bus.on('bar', () => ( - Promise.reject(error) - )); - bus.emit('bar', true); + it('emits an "error" event if a synchronous error is thrown in a hook', () => { + const error = new Error('error'); + bus.hook('active', () => { + throw error; + }); + bus.on('foo', () => {}); + expect(onError).toHaveBeenCalledWith({ + error, + event: 'active' + }); + }); + + it('emits "error" when a hook returns a promise that rejects', async () => { + const error = new Error('error'); + bus.hook('active', () => ( + Promise.reject(error) + )); + bus.on('foo', () => {}); - // Wait for promises to be processed - await Promise.resolve(); + // Wait for promises to be processed + await Promise.resolve(); - expect(onError).toHaveBeenCalledWith({ - error, - event: 'bar' + expect(onError).toHaveBeenCalledWith({ + error, + event: 'active' + }); }); });