Skip to content
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

Observe PurchaseHandler when owned externally #4097

Merged
merged 1 commit into from
Aug 8, 2024

Conversation

jamesrb1
Copy link
Contributor

Josh kindly fixed a problem while I was away where the purchase handler would get clobbered when the paywall was re-rendered. This PR restores avoiding having one struct owned by two @StateObjects at the same time.

@jamesrb1 jamesrb1 added the bug label Jul 22, 2024
@jamesrb1 jamesrb1 requested a review from joshdholtz July 22, 2024 21:07
@jamesrb1 jamesrb1 marked this pull request as ready for review July 22, 2024 21:08
@jamesrb1 jamesrb1 requested a review from a team July 22, 2024 21:24
@jamesrb1 jamesrb1 added pr:fix A bug fix and removed bug labels Jul 22, 2024
Copy link
Member

@joshdholtz joshdholtz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jamesrb1 I think this looks good!

Does this still work with the overlay use case from the #4034?

Also, was there an example app use case where this fix was needed? Like where we observed some weird behavior? 🤔 Just want to make sure for documentation purposes incase we come across this again the in the future 😇

@jamesrb1
Copy link
Contributor Author

Does this still work with the #4034 (comment) from the #4034?

Yes!

Screen.Recording.2024-07-26.at.11.33.47.AM.mov

Also, was there an example app use case where this fix was needed? Like where we observed some weird behavior? 🤔 Just want to make sure for documentation purposes incase we come across this again the in the future 😇

No, I've observed no ill-behavior from having one object referenced by two @StateObject properties. Everything I've read says one thing should own it, everything else should observe it. If there are zero @StateObject references, you risk unexpected behaviour and crashes. But I have not seen anything that addresses the case of having multiple @StateObjects, but since that goes against the expected usage, I think the only safe way to consider this is that it's undefined behavior. (If the "second" view that owns it as a @StateObject is destroyed, does that cause the object to be destroyed too, when something else is still owning it? Or does this use a reference counting setup where it would still be OK? I don't know, and I don't know if it's defined anywhere so what it does could change in the future.)

@jamesrb1 jamesrb1 merged commit 3c08475 into main Aug 8, 2024
4 checks passed
@jamesrb1 jamesrb1 deleted the james/alternate-fix-for-clobbered-purchase-handler branch August 8, 2024 16:36
joshdholtz added a commit that referenced this pull request Aug 22, 2024
**This is an automatic release.**

### New Features
* Price rounding logic (#4132) via James Borthwick (@jamesrb1)
### Bugfixes
* [Customer Center] Migrate to List style (#4190) via Cody Kerns
(@codykerns)
* [Paywalls] Improve locale consistency (#4158) via Josh Holtz
(@joshdholtz)
* Set Paywalls Tester deployment target to iOS 15 (#4196) via James
Borthwick (@jamesrb1)
* [Customer Center] Hide Contact Support button if URL can't be created
(#4192) via Cesar de la Vega (@vegaro)
* Fix the setting for SKIP_INSTALL in Xcode project (#4195) via Andy
Boedo (@aboedo)
* [Customer Center] Improving customer center buttons (#4165) via Cody
Kerns (@codykerns)
* Revert workaround for iOS 18 beta 5 SwiftUI crash (#4173) via Mark
Villacampa (@MarkVillacampa)
* [Paywalls] Make iOS version calculation lazy (#4163) via Mark
Villacampa (@MarkVillacampa)
* [Paywalls] Observe `PurchaseHandler` when owned externally (#4097) via
James Borthwick (@jamesrb1)
### Other Changes
* [Customer Center] Clean up colors in WrongPlatformView and
NoSubscriptionsView (#4204) via Cesar de la Vega (@vegaro)
* Fix failing `all-tests` and retry more flaky tests (#4188) via Josh
Holtz (@joshdholtz)
* Compatibility content unavailable improvements (#4197) via James
Borthwick (@jamesrb1)
* Create lane to enable customer center (#4191) via Cesar de la Vega
(@vegaro)
* XCFramework artifacts in CircleCI (#4189) via Andy Boedo (@aboedo)
* [Customer Center] CustomerCenterViewModel checks whether the app is
the latest version (#4169) via JayShortway (@JayShortway)
* export RevenueCatUI xcframework (#4172) via Andy Boedo (@aboedo)
* Corrects references from ManageSubscriptionsButtonStyle to
ButtonsStyle. (#4186) via JayShortway (@JayShortway)
* Speed up carthage installation tests (#4184) via Andy Boedo (@aboedo)
* Customer center improvements (#4166) via James Borthwick (@jamesrb1)
* replace `color(from colorInformation:)` global with extension (#4183)
via Andy Boedo (@aboedo)
* Fix tests in main (#4174) via Andy Boedo (@aboedo)
* Enable customer center tests (#4171) via James Borthwick (@jamesrb1)
* [Customer Center] Initial implementation (#3967) via Cesar de la Vega
(@vegaro)

---------

Co-authored-by: Josh Holtz <me@joshholtz.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:fix A bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants