-
Notifications
You must be signed in to change notification settings - Fork 43
fix: Resolve circular dependencies in the SDK package #3361
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
3f5566d to
1bb9784
Compare
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.
Pull request overview
This PR resolves circular dependencies in the SDK by implementing Symbol-based dependency injection tokens for the Resends and Subscriber classes. This approach eliminates the need for delay() wrappers and provides better ESM compatibility for future migration.
Changes:
- Introduces Symbol-based injection tokens for
ResendsandSubscriberclasses - Updates dependency injection to use tokens instead of
delay()wrappers - Converts class imports to type-only imports where the class is now injected via token
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/sdk/src/tokens.ts | Defines Symbol-based injection tokens for Resends and Subscriber |
| packages/sdk/src/setupTsyringe.ts | Registers Resends and Subscriber classes with their respective tokens using ContainerScoped lifecycle |
| packages/sdk/src/subscribe/MessagePipelineFactory.ts | Replaces delay(() => Resends) with token-based injection and adds type-only import for Resends |
| packages/sdk/src/encryption/SubscriberKeyExchange.ts | Updates Subscriber injection to use token instead of direct class reference and converts to type-only import |
| packages/sdk/src/StreamrClient.ts | Updates resolution of Resends and Subscriber to use tokens instead of class references |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Resolve circular dependencies in the SDK's dependency injection (DI) container by using token-based injection instead of direct class references. This eliminates the need for
delay()wrappers forResendsandSubscriberclasses.Important
This change is necessary for future ESM compatibility. The current
delay(() => Class)approach relies on CommonJS module semantics where circular imports can be partially resolved at runtime. In ESM, module bindings are evaluated differently anddelay()won't be sufficient to break circular dependency cycles.Token-based injection provides a clean solution that works in both module systems.
Changes
tokens.tswith Symbol-based injection tokens forResendsandSubscribersetupTsyringe.tsto registerResendsandSubscriberclasses with the DI container using tokens (withContainerScopedlifecycle)@scoped(Lifecycle.ContainerScoped)decorators with@injectable()forResendsandSubscriberclasses (lifecycle is now specified during token registration)MessagePipelineFactoryto use token injection forResendsinstead ofdelay(() => Resends)SubscriberKeyExchangeto use token injection forSubscriber(previously injected directly withoutdelay())StreamrClientto resolveSubscriberandResendsusing tokens instead of class referencesLimitations and future improvements
ResendsandSubscriberare currently registered via tokens; other classes with circular dependency potential could be migrated to this pattern in the futuredelay()wrapper is still used forStreamRegistryandGroupKeyManagerinMessagePipelineFactory; these could be converted to token injection for consistency