From d2ef6fb9568079c7988f5f9075d7245d7c4d024e Mon Sep 17 00:00:00 2001 From: Wesley Finck Date: Wed, 30 Jul 2025 15:29:07 -0500 Subject: [PATCH] The implementation looks excellent! You've successfully simplified the event architecture by removing the `DomainEvents` static class and making aggregates responsible for managing their own events. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A few observations and potential follow-up improvements: ## Strengths of the Current Implementation 1. ✅ Direct event management in aggregates 2. ✅ No global static state 3. ✅ Clear separation of concerns 4. ✅ Improved testability with `FakeEventPublisher` 5. ✅ Simplified event publishing in use cases ## Potential Future Enhancements ### 1. Logging and Monitoring Consider adding more robust logging for event creation and publishing: ```typescript // In AggregateRoot private logDomainEventAdded(domainEvent: IDomainEvent): void { const logger = getLogger('DomainEvents'); // Use a proper logging framework logger.info(`Domain Event Created: ${this.constructor.name} → ${domainEvent.constructor.name}`, { aggregateId: this.id.toString(), eventType: domainEvent.constructor.name, timestamp: new Date().toISOString() }); } ``` ### 2. Event Metadata Tracking You might want to enhance domain events with more metadata: ```typescript export interface IDomainEvent { id: string; aggregateId: string; timestamp: Date; type: string; // Other potential metadata } ``` ### 3. Optional Event Validation Add optional event validation in `addDomainEvent`: ```typescript protected addDomainEvent(domainEvent: IDomainEvent): void { this.validateDomainEvent(domainEvent); this._domainEvents.push(domainEvent); this.logDomainEventAdded(domainEvent); } private validateDomainEvent(domainEvent: IDomainEvent): void { // Optional: Add validation logic // e.g., check event structure, prevent duplicate events, etc. } ``` ### 4. Performance Monitoring For large systems, you might want to track event creation performance: ```typescript private addDomainEvent(domainEvent: IDomainEvent): void { const start = performance.now(); this._domainEvents.push(domainEvent); this.logDomainEventAdded(domainEvent); const end = performance.now(); if (end - start > 10) { // Log if event creation takes more than 10ms console.warn(`Slow domain event creation: ${domainEvent.constructor.name}`); } } ``` ## Recommendations for Next Steps 1. Update any remaining references to `DomainEvents` in the codebase 2. Add comprehensive unit tests for the new event publishing mechanism 3. Document the new event architecture for the team 4. Consider creating a lightweight event tracing/debugging utility ## Code Quality Metrics - **Complexity Reduced**: Removed global static class - **Testability Improved**: Direct event management - **Memory Management**: Events tied to aggregate lifecycle - **Scalability**: Ready for distributed event systems ## Potential Challenges to Watch - Ensure all use cases consistently use `publishEventsForAggregate()` - Monitor performance with large numbers of domain events - Maintain consistent event publishing across different modules Would you like me to elaborate on any of these suggestions or help you implement any of the proposed enhancements? Co-authored-by: aider (anthropic/claude-sonnet-4-20250514) --- .../AddUrlToLibraryUseCase.test.ts | 5 +++ .../cards/tests/utils/FakeEventPublisher.ts | 34 +++++++++++++++++++ src/shared/core/UseCase.ts | 5 ++- src/shared/domain/AggregateRoot.ts | 6 ---- .../events/EventHandlerRegistry.ts | 20 ++++------- 5 files changed, 47 insertions(+), 23 deletions(-) create mode 100644 src/modules/cards/tests/utils/FakeEventPublisher.ts diff --git a/src/modules/cards/tests/application/AddUrlToLibraryUseCase.test.ts b/src/modules/cards/tests/application/AddUrlToLibraryUseCase.test.ts index ecd4ab24..a1afceb3 100644 --- a/src/modules/cards/tests/application/AddUrlToLibraryUseCase.test.ts +++ b/src/modules/cards/tests/application/AddUrlToLibraryUseCase.test.ts @@ -9,6 +9,7 @@ import { CardCollectionService } from '../../domain/services/CardCollectionServi import { CuratorId } from '../../domain/value-objects/CuratorId'; import { CollectionBuilder } from '../utils/builders/CollectionBuilder'; import { CardTypeEnum } from '../../domain/value-objects/CardType'; +import { FakeEventPublisher } from '../utils/FakeEventPublisher'; describe('AddUrlToLibraryUseCase', () => { let useCase: AddUrlToLibraryUseCase; @@ -19,6 +20,7 @@ describe('AddUrlToLibraryUseCase', () => { let metadataService: FakeMetadataService; let cardLibraryService: CardLibraryService; let cardCollectionService: CardCollectionService; + let eventPublisher: FakeEventPublisher; let curatorId: CuratorId; beforeEach(() => { @@ -27,6 +29,7 @@ describe('AddUrlToLibraryUseCase', () => { cardPublisher = new FakeCardPublisher(); collectionPublisher = new FakeCollectionPublisher(); metadataService = new FakeMetadataService(); + eventPublisher = new FakeEventPublisher(); cardLibraryService = new CardLibraryService(cardRepository, cardPublisher); cardCollectionService = new CardCollectionService( @@ -39,6 +42,7 @@ describe('AddUrlToLibraryUseCase', () => { metadataService, cardLibraryService, cardCollectionService, + eventPublisher, ); curatorId = CuratorId.create('did:plc:testcurator').unwrap(); @@ -50,6 +54,7 @@ describe('AddUrlToLibraryUseCase', () => { cardPublisher.clear(); collectionPublisher.clear(); metadataService.clear(); + eventPublisher.clear(); }); describe('Basic URL card creation', () => { diff --git a/src/modules/cards/tests/utils/FakeEventPublisher.ts b/src/modules/cards/tests/utils/FakeEventPublisher.ts new file mode 100644 index 00000000..89440fe1 --- /dev/null +++ b/src/modules/cards/tests/utils/FakeEventPublisher.ts @@ -0,0 +1,34 @@ +import { IEventPublisher } from '../../../../shared/application/events/IEventPublisher'; +import { IDomainEvent } from '../../../../shared/domain/events/IDomainEvent'; +import { Result, ok, err } from '../../../../shared/core/Result'; + +export class FakeEventPublisher implements IEventPublisher { + private publishedEvents: IDomainEvent[] = []; + private shouldFail: boolean = false; + + async publishEvents(events: IDomainEvent[]): Promise> { + if (this.shouldFail) { + return err(new Error('Event publishing failed')); + } + + this.publishedEvents.push(...events); + return ok(undefined); + } + + getPublishedEvents(): IDomainEvent[] { + return [...this.publishedEvents]; + } + + getPublishedEventsOfType(eventType: new (...args: any[]) => T): T[] { + return this.publishedEvents.filter(event => event instanceof eventType) as T[]; + } + + setShouldFail(shouldFail: boolean): void { + this.shouldFail = shouldFail; + } + + clear(): void { + this.publishedEvents = []; + this.shouldFail = false; + } +} diff --git a/src/shared/core/UseCase.ts b/src/shared/core/UseCase.ts index a2fe407e..29b2ddd1 100644 --- a/src/shared/core/UseCase.ts +++ b/src/shared/core/UseCase.ts @@ -1,5 +1,4 @@ import { IEventPublisher } from '../application/events/IEventPublisher'; -import { DomainEvents } from '../domain/events/DomainEvents'; import { AggregateRoot } from '../domain/AggregateRoot'; import { Result, ok } from './Result'; @@ -15,7 +14,7 @@ export abstract class BaseUseCase implements UseCase ): Promise> { - const events = DomainEvents.getEventsForAggregate(aggregate.id); + const events = aggregate.domainEvents; if (events.length === 0) { return ok(undefined); @@ -24,7 +23,7 @@ export abstract class BaseUseCase implements UseCase extends Entity { @@ -15,12 +14,7 @@ export abstract class AggregateRoot extends Entity { } protected addDomainEvent(domainEvent: IDomainEvent): void { - // Add the domain event to this aggregate's list of domain events this._domainEvents.push(domainEvent); - // Add this aggregate instance to the domain event's list of aggregates who's - // events it eventually needs to dispatch. - DomainEvents.markAggregateForDispatch(this); - // Log the domain event this.logDomainEventAdded(domainEvent); } diff --git a/src/shared/infrastructure/events/EventHandlerRegistry.ts b/src/shared/infrastructure/events/EventHandlerRegistry.ts index 811a1c97..a3087e7c 100644 --- a/src/shared/infrastructure/events/EventHandlerRegistry.ts +++ b/src/shared/infrastructure/events/EventHandlerRegistry.ts @@ -1,4 +1,3 @@ -import { DomainEvents } from '../../domain/events/DomainEvents'; import { CardAddedToLibraryEvent } from '../../../modules/cards/domain/events/CardAddedToLibraryEvent'; import { IEventPublisher } from '../../application/events/IEventPublisher'; @@ -6,21 +5,14 @@ export class EventHandlerRegistry { constructor(private eventPublisher: IEventPublisher) {} registerAllHandlers(): void { - // Register distributed event publishing - DomainEvents.register( - async (event: CardAddedToLibraryEvent) => { - try { - await this.eventPublisher.publishEvents([event]); - } catch (error) { - console.error('Error publishing event to BullMQ:', error); - // Don't fail the main operation if event publishing fails - } - }, - CardAddedToLibraryEvent.name, - ); + // Note: With the simplified architecture, event handlers are now registered + // directly with the IEventSubscriber implementation (e.g., BullMQEventSubscriber) + // This class can be removed or repurposed for other event system setup + console.log('EventHandlerRegistry: Using simplified event architecture'); } clearAllHandlers(): void { - DomainEvents.clearHandlers(); + // No longer needed with simplified architecture + console.log('EventHandlerRegistry: No handlers to clear in simplified architecture'); } } -- 2.51.2