-
Notifications
You must be signed in to change notification settings - Fork 14
feat: add updateMemberProfile and emit methods to FederationSDK exports #298
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThe PR introduces EventEmitterService as a dependency in FederationSDK, adds a new public method updateMemberProfile that delegates to roomService, and exposes an emit method that delegates to eventEmitterService. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
394864d to
41f756c
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #298 +/- ##
==========================================
- Coverage 52.61% 52.43% -0.19%
==========================================
Files 96 96
Lines 12617 12669 +52
==========================================
+ Hits 6639 6643 +4
- Misses 5978 6026 +48 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/federation-sdk/src/sdk.ts (1)
305-307: Verify that exposing raw event emission is the intended API design.The
emitmethod exposes the underlying event emitter directly, which allows consumers to emit arbitrary events. While the implementation is technically correct and follows the established delegation pattern, this design choice warrants verification:
- Flexibility vs. Control: Direct emit access provides maximum flexibility but bypasses the SDK's controlled API surface.
- Type Safety: Without event name constraints, consumers can emit any event string, which may not be desirable.
- Alternative: Consider whether specific event-emitting methods (e.g.,
emitRoomStateChanged,emitMemberJoined) would provide better type safety and API clarity.Additionally, consider adding JSDoc documentation to describe:
- What events can/should be emitted
- When consumers should use this method
- Expected event payload structures
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/federation-sdk/src/sdk.ts(4 hunks)
🔇 Additional comments (3)
packages/federation-sdk/src/sdk.ts (3)
8-8: LGTM!The import follows the established pattern and is correctly placed among other service imports.
41-41: LGTM!The constructor parameter follows the established dependency injection pattern using tsyringe.
104-108: LGTM!The
updateMemberProfilemethod correctly delegates to the room service and follows the established delegation pattern used throughout the SDK.
Summary by CodeRabbit