feat(event): Module providers architecture #14532
Conversation
🦋 Changeset detectedLatest commit: 66e42d3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 78 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 8 Skipped Deployments
|
|
|
@claude review this |
PR Review: Event Module Provider ArchitectureSummaryThis PR introduces a significant architectural improvement by implementing a provider-based event module system. The change migrates from directly using event-bus-local/event-bus-redis to a unified event module with pluggable providers. Overall, the implementation is well-structured and follows Medusa's conventions, but there are several critical issues that need attention. 🚨 Critical Issues1. Test Script Modified to Run Single TestLocation: - "test:integration": "jest --no-cache --bail --detectOpenHandles --forceExit --logHeapUsage",
+ "test:integration": "jest --no-cache --bail --detectOpenHandles --forceExit --logHeapUsage -- __tests__/product/admin/product-export.spec.ts",Issue: The test script has been modified to only run a single test file. This appears to be a debugging change that should NOT be committed to the codebase. Fix: Revert this line to run all integration tests.
|
|
@claude review this |
PR Review: Event Module Provider ArchitectureThis PR introduces a significant architectural improvement by creating a new event module with a provider-based architecture, allowing pluggable event providers (local and Redis). Here's my detailed review: ✅ Strengths1. Architecture & Design
2. Code Quality
3. Feature Completeness
4. Testing
|
|
@claude review this |
Code Review - PR #14532: Event Module Providers ArchitectureSummaryThis PR introduces a significant architectural improvement by refactoring the event bus into a modular provider-based system. The new ✅ Strengths1. Excellent Architecture Design
2. Strong Type Safety
3. Good Documentation
4. Backward Compatibility
5. Feature Parity & Enhancements
🔍 Issues & Concerns1. Critical: Redis Connection Error Handling
|
packages/modules/providers/events-redis/src/services/events-redis.ts
Outdated
Show resolved
Hide resolved
…edusa into feat/event-module-w-providers
…edusa into feat/event-module-w-providers
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
olivermrbl
left a comment
olivermrbl
left a comment
There was a problem hiding this comment.
Overall LGTM, nice work!
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@olivermrbl should be good for a last review to be sure 👍 |
olivermrbl
left a comment
olivermrbl
left a comment
There was a problem hiding this comment.
LGTM, two non-blocking nits
| * Redis-based event provider using BullMQ for reliable event processing. | ||
| * Supports priority, retry, and distributed event handling. | ||
| */ | ||
| export class RedisEventProvider extends EventsUtils.AbstractEventsProvider<EventRedisProviderOptions> { |
There was a problem hiding this comment.
| export class RedisEventProvider extends EventsUtils.AbstractEventsProvider<EventRedisProviderOptions> { | |
| export class RedisEventsProvider extends EventsUtils.AbstractEventsProvider<EventRedisProviderOptions> { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@claude review this pleaase, be succinte and to the point. I d like an overall assesment as well |
Pull Request Review: Event Module with Provider ArchitectureSummaryThis PR introduces a significant architectural improvement by implementing a provider-based events module that replaces the existing event-bus architecture. The implementation is well-structured and maintains backward compatibility while providing a more extensible foundation. ✅ StrengthsArchitecture & Design
Redis Provider Implementation
Testing
🔍 Issues & ConcernsCritical Issues1. Race Condition in Interceptor Calls (events-redis.ts:311-322)Severity: High The interceptor calls are not awaited: groupedEvents.map((jobData) => {
// ...
this.callInterceptors(message as any, {...}) // No await!
})Impact: Interceptors may not complete before events are emitted, defeating their purpose. Fix: Use await promiseAll(
groupedEvents.map((jobData) => {
const message = {...}
return this.callInterceptors(message, {...})
})
)2. Unsafe Type Assertion (events-redis.ts:318)Severity: Medium this.callInterceptors(message as any, {...})Impact: Bypasses TypeScript type safety, could hide type mismatches. Fix: Properly type the message construction or use a type guard. 3. Error Handling in Abstract Provider (abstract-event-provider.ts:349-356)Severity: Medium protected async callInterceptors<T = unknown>(...) {
Array.from(this.interceptorSubscribers_).map(async (interceptor) => {
try {
await interceptor(message, context)
} catch (error) {
this.logger_.error("Error in event bus interceptor:", error)
}
})
}Issues:
Fix: protected async callInterceptors<T = unknown>(...) {
await promiseAll(
Array.from(this.interceptorSubscribers_).map(async (interceptor) => {
try {
await interceptor(message, context)
} catch (error) {
this.logger_.error("Error in event bus interceptor:", error)
}
})
)
}Medium Issues4. Inconsistent subscriberId Generation (event-module.ts:98)Severity: Medium const subscriberId = context?.subscriberId ?? `${eventName.toString()}-${ulid()}`This is good, but the local provider (local.ts:221) also has fallback logic that could conflict: const subscriberId = context?.subscriberId ?? (subscriber as any).subscriberIdRecommendation: Centralize the subscriberId generation logic (which appears to be the intent based on commit 94593db). 5. Memory Leak Potential in Local Provider (local.ts:18-19)Severity: Low-Medium const eventEmitter = new EventEmitter()
eventEmitter.setMaxListeners(Infinity)Concern: Shared EventEmitter across all instances with unlimited listeners could lead to memory issues. Recommendation: Consider instance-scoped EventEmitters or monitoring listener counts. 6. Missing Validation in Redis Provider (events-redis.ts:186-197)Severity: Low Priority validation only warns but continues: if (finalOptions.priority != undefined && ...) {
this.logger_.warn(`Invalid priority value...`)
finalOptions.priority = EventPriority.DEFAULT
}Recommendation: Consider throwing an error for invalid priorities in development mode. Performance Considerations7. Grouped Events Serialization (events-redis.ts:133)groupedEvents = JSON.parse(JSON.stringify(groupedEvents))Concern: Deep clone of potentially large event arrays. This could be expensive. Recommendation: Consider if deep clone is necessary or if shallow copy would suffice. 8. Redis LRANGE Without Pagination (events-redis.ts:301-304).lrange(`staging:${eventGroupId}`, 0, -1)Concern: Fetches all events in a group without pagination. Could be problematic for large event groups. Recommendation: Add pagination or document maximum recommended events per group. Code Quality9. Commented Code (event-bus-local.ts:11-14, event-bus-redis.ts:31-35)// console.warn(
// "[DEPRECATION WARNING] ..."
// )Recommendation: Either enable these warnings or remove commented code. Users should be informed about deprecations. 10. Error Message Clarity (event-provider.ts:42-44)const errMessage = `
Unable to retrieve the event provider with id: ${providerId}
Please make sure that the provider is registered...`Recommendation: Include available provider IDs in the error message to aid debugging. Security✅ No SQL injection risks (uses parameterized Redis operations) Test CoverageGood:
Missing:
📝 RecommendationsHigh Priority
Medium Priority
Low Priority
📊 Overall AssessmentCode Quality: 8/10 Overall: This is a well-architected improvement that significantly enhances the events system. The main concerns are around async/await patterns with interceptors and test coverage for the Redis provider. Once the critical issues are addressed, this will be a solid foundation for the events architecture. The backward compatibility approach is excellent and will allow for smooth migration. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
what
Introduce the new event module supporting provider architecture natively. This pr provide a backward compatibility approach and set the new module as the default.
The module comes with the local in built provider as well as a separate redis provider.]
This will allow more provider to be created to answer different needs
Note
Introduces a provider-based Events module and makes it the default event system with built-in local and optional Redis providers.
@medusajs/eventsmodule with provider architecture,IEventsModuleService/IEventsProvidertypes, interceptors, grouped events APIs, andAbstractEventsProvider@medusajs/events-redis(BullMQ/ioredis) with priority/retry, lifecycle hooks, and grouped event handling@medusajs/medusa/eventsandevents-redis; marksevent-bus-local/event-bus-redisas deprecated shimsevent_busto@medusajs/medusa/eventswithprovidersoption; updates utils and module resolutiongetProvider('local')) and ensures compatibility in product import/export flowsWritten by Cursor Bugbot for commit 66e42d3. This will update automatically on new commits. Configure here.