-
Notifications
You must be signed in to change notification settings - Fork 3
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
Fixes the PurchasesDelegate being deallocated on iOS #214
Conversation
… it being deallocated unintentionally.
* **Note:** If your delegate is not a singleton, make sure you set this back to null when | ||
* you're done to avoid memory leaks. For instance, if your delegate is tied to a screen, set | ||
* this to null when the user navigates away from the screen. |
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 already applied to Android, so would already have been useful to call out, but now it applies to both platforms.
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.
Hmm it's true that in iOS delegates are much more commonly held weakly... But yeah, since this is KMP, I think this makes sense 👍
internal fun PurchasesDelegate?.toRcPurchasesDelegate(): RCPurchasesDelegateProtocol? = | ||
this?.let { PurchasesDelegateWrapper(it) } | ||
.also { PurchasesDelegateStrongReference.delegate = it } |
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 is changed so that when the delegate
is set to null, the strong reference is cleared.
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.
Just one comment about tying the lifecycle of this to the Purchases
object. Looks good otherwise!
* **Note:** If your delegate is not a singleton, make sure you set this back to null when | ||
* you're done to avoid memory leaks. For instance, if your delegate is tied to a screen, set | ||
* this to null when the user navigates away from the screen. |
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.
Hmm it's true that in iOS delegates are much more commonly held weakly... But yeah, since this is KMP, I think this makes sense 👍
* extend `NSObject`. See also | ||
* [KT67930](https://youtrack.jetbrains.com/issue/KT-67930/Getting-Crash-while-building-KMM-project-with-XCode-15.3#focus=Comments-27-9796215.0-0) | ||
*/ | ||
private object PurchasesDelegateStrongReference { |
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.
Hmm it would be great if we could at least tie it to the Purchases
object lifecycle... So if the Purchases
object is cleared, this is also cleared... Wdyt?
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.
Oh great suggestion! This should do it: cc4f72b.
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.
Just one more comment, but looks good!
* [KT67930](https://youtrack.jetbrains.com/issue/KT-67930/Getting-Crash-while-building-KMM-project-with-XCode-15.3#focus=Comments-27-9796215.0-0) | ||
*/ | ||
private object PurchasesDelegateStrongReference { | ||
var delegate: RCPurchasesDelegateProtocol? = null |
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.
Would it be possible to just hold this inside the Purchases.ios
class instead? Just to make sure if it gets dereferenced (without calling the close
function, we also clear it. (honestly this is an edge case, since usually Purchases
would live forever, but just in case)
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.
Ah yes that makes way more sense haha! 1810b44
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.
Looks great now, thanks for doing the changes :)
Bug
The
PurchasesDelegate
sometimes doesn't get called on iOS.Cause
On the iOS platform side, the backing field of
Purchases.delegate
,Purchases.privateDelegate
, is aweak var
. On the multiplatform side, we wrap the actual delegate inPurchasesDelegateWrapper
to conform toRCPurchasesDelegateProtocol
. Since that is a private implementation detail, the only reference held to our wrapper is theweak var
. This means that it gets deallocated the first chance it gets.Fix
We keep a strong reference to our
PurchasesDelegateWrapper
. This matches the Android behavior, which keeps a strong reference on the platform side already.Fixes #203