-
Notifications
You must be signed in to change notification settings - Fork 336
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
v4: Fix crash in iOS 11-12 when using MainActor #4718
base: release/4.43.3
Are you sure you want to change the base?
Changes from all commits
5696e6b
7cdb21f
64b17b9
74722be
bd8a70b
b0d9788
a5f2aed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,7 +27,7 @@ class StoreKitRequestFetcher: NSObject { | |
|
||
private let requestFactory: ReceiptRefreshRequestFactory | ||
private var receiptRefreshRequest: SKRequest? | ||
private var receiptRefreshCompletionHandlers: [@MainActor @Sendable () -> Void] | ||
private var receiptRefreshCompletionHandlers: [@Sendable () -> Void] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to understand, we shouldn't be using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. exclusively in v4 of the iOS SDK. This is because it has a deployment target of iOS 11 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. v5 is iOS 13+ so we can use it there |
||
private let operationDispatcher: OperationDispatcher | ||
|
||
init(requestFactory: ReceiptRefreshRequestFactory = ReceiptRefreshRequestFactory(), | ||
|
@@ -38,7 +38,7 @@ class StoreKitRequestFetcher: NSObject { | |
self.receiptRefreshCompletionHandlers = [] | ||
} | ||
|
||
func fetchReceiptData(_ completion: @MainActor @Sendable @escaping () -> Void) { | ||
func fetchReceiptData(_ completion: @Sendable @escaping () -> Void) { | ||
self.operationDispatcher.dispatchOnWorkerThread { | ||
self.receiptRefreshCompletionHandlers.append(completion) | ||
|
||
|
@@ -94,7 +94,7 @@ private extension StoreKitRequestFetcher { | |
self.receiptRefreshCompletionHandlers = [] | ||
|
||
for handler in completionHandlers { | ||
self.operationDispatcher.dispatchOnMainActor { | ||
self.operationDispatcher.dispatchOnMainThread { | ||
handler() | ||
} | ||
} | ||
|
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 one keeps the
@MainActor
. I'm guessing it's not used anymore in this v4. If so, perhaps we should remove it completely to prevent potential future errors if we need to make a new release of v4?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 think the concern around that one was about making further changes to the API - like, if your code relies on this being
@MainActor
, you might have broken compilation after upgrading.I mean, the original problem was to add
@MainActor
in a non-minor anyway, but kinda don't want to repeat the same issue twiceThere 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.
might as well kill the extra space
so it doesn't even show up in the diff tho 😅
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.
Yeah, I think it makes sense 👍