-
Notifications
You must be signed in to change notification settings - Fork 0
Modify the entities to hold a strong Chat reference #39
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
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
| @@ -27,15 +27,13 @@ class ChatAdapter { | |||
| } | |||
|
|
|||
| static func associate(chat: ChatImpl, with kotlinChat: PubNubChat.ChatImpl) { | |||
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.
This class maintains bidirectional associations between Swift's ChatImpl and KMP's PubNubChat.ChatImpl instances. It's required because KMP entities (Channel, User, Message) must be initialized with KMP Chat references, and these entities store strong references back to Chat within the KMP layer
| if !associations.contains(where: { !$0.isEmpty() && $0.chat !== chat && $0.kotlinChat !== kotlinChat }) { | ||
| associations.append(.init(chat: chat, kotlinChat: kotlinChat)) | ||
| } | ||
| queue.sync { |
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.
My understanding is that the new code always appends, so you definitely can have multiple Association objects for the same kotlinChat or chat.
Given your requirement:
maintains bidirectional associations between Swift's ChatImpl and KMP's ChatImpl
you almost certainly want:
At most one association per ChatImpl
At most one association per PubNubChat.ChatImpl
Otherwise map(chat:) just returns the first match and leaves dangling duplicates around.
Here’s a version aligned with the “strict 1–1 mapping” idea, using your new serial queue with sync:
static func associate(chat: ChatImpl, with kotlinChat: PubNubChat.ChatImpl) {
queue.sync {
// Drop dead associations first
associations.removeAll { $0.isEmpty() }
// If there is an existing association for either side, reuse/update it
if let existing = associations.first(where: {
!$0.isEmpty() && ($0.chat === chat || $0.kotlinChat === kotlinChat)
}) {
// You’ll need these setters or make _chat/_kotlinChat internal
existing._chat = chat
existing._kotlinChat = kotlinChat
} else {
associations.append(.init(chat: chat, kotlinChat: kotlinChat))
}
}
}
Then you’d need to relax Association a bit:
class Association {
// swiftlint:disable:next force_unwrapping
var chat: ChatImpl { _chat! }
// swiftlint:disable:next force_unwrapping
var kotlinChat: PubNubChat.ChatImpl { _kotlinChat! }
// make these internal(set) so associate() can adjust them
weak var _chat: ChatImpl?
weak var _kotlinChat: PubNubChat.ChatImpl?
init(chat: ChatImpl, kotlinChat: PubNubChat.ChatImpl) {
_chat = chat
_kotlinChat = kotlinChat
}
func isEmpty() -> Bool {
_chat == nil || _kotlinChat == nil
}
}
This gives you:
1–1 mapping between Swift and KMP chat instances.
Reuse/repair of associations when one side is recreated but the other is still alive.
No growth of duplicates over time.
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.
I completely got rid of these associations and ensured entities hold a strong reference to their Chat instance
|
@pubnub-release-bot release as 0.33.0 |
4d730cd to
38c767f
Compare
|
🚀 Release successfully completed 🚀 |
fix(chat): modify the entities to hold a strong Chat reference