[in_app_purchase_storekit] Migrate StoreKit1 core to Swift - #12642
[in_app_purchase_storekit] Migrate StoreKit1 core to Swift#12642danielleon-cmd wants to merge 4 commits into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Pigeon's Swift codegen defaults to structs, but has the |
96ca1bc to
359783f
Compare
Ports FIAObjectTranslator, FIAPReceiptManager, FIAPRequestHandler, FIAPPaymentQueueDelegate, FIATransactionCache, and their protocol shims (FLTPaymentQueueProtocol, FLTTransactionCacheProtocol, FLTMethodChannelProtocol, FLTRequestHandlerProtocol, FLTPaymentQueueHandlerProtocol) from Objective-C to Swift, completing the migration started by the existing FIAPaymentQueueHandler.swift. Pigeon-generated messages.g.m/.h stay Objective-C, since Pigeon's Swift codegen can't represent the self-referential SKPaymentTransactionMessage struct. Also ports the package's Objective-C test doubles (Stubs.h/Stubs.m) to Swift, since the RunnerTests target has no way to see the plugin's new Swift-only declarations from Objective-C in this repo's SPM-based plugin setup. Verified via the full native macOS unit test suite (76/76 passing). iOS native-test could not be run in this environment (no iOS code signing certificate configured on this machine — confirmed this is a pre-existing, unrelated limitation by reproducing it against an untouched plugin), but iOS shares the exact same Swift source as macOS. Part of flutter/flutter#102679. FPOCTSMP-29
359783f to
f28715e
Compare
…-in-app-purchase-storekit-swift # Conflicts: # packages/in_app_purchase/in_app_purchase_storekit/CHANGELOG.md
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the StoreKit 1 core classes and protocols from Objective-C to Swift. The review feedback identifies several critical issues in FIAObjectTranslator.swift where casting optional values directly to Any can result in Optional<Any>.none being stored in dictionaries, which causes runtime crashes when bridged to NSDictionary. The reviewer suggests using ?? NSNull() to safely represent nil values, using standard APIs like originalTransaction instead of custom extensions, and adopting more idiomatic Swift dictionary subscripts in FIATransactionCache.swift.
| public static func getMapFrom(_ product: SKProduct) -> [String: Any] { | ||
| return [ | ||
| "discounts": getMapArrayFrom(product.discounts), | ||
| "introductoryPrice": product.introductoryPrice.map { getMapFrom($0) } as Any, | ||
| "localizedDescription": product.localizedDescription, | ||
| "localizedTitle": product.localizedTitle, | ||
| "productIdentifier": product.productIdentifier, | ||
| "price": product.price.description, | ||
| "subscriptionGroupIdentifier": product.subscriptionGroupIdentifier as Any, | ||
| "subscriptionPeriod": product.subscriptionPeriod.map { getMapFrom($0) } as Any, | ||
| "priceLocale": getMapFrom(product.priceLocale), | ||
| ] | ||
| } |
There was a problem hiding this comment.
Casting optional values directly to Any (e.g., product.introductoryPrice.map { getMapFrom($0) } as Any) can result in Optional<Any>.none being stored in the dictionary. When this dictionary is bridged to NSDictionary (which occurs during Flutter platform channel serialization), it will cause a runtime crash because NSDictionary cannot contain nil values. Use ?? NSNull() to safely represent nil values as NSNull and prevent crashes.
| public static func getMapFrom(_ product: SKProduct) -> [String: Any] { | |
| return [ | |
| "discounts": getMapArrayFrom(product.discounts), | |
| "introductoryPrice": product.introductoryPrice.map { getMapFrom($0) } as Any, | |
| "localizedDescription": product.localizedDescription, | |
| "localizedTitle": product.localizedTitle, | |
| "productIdentifier": product.productIdentifier, | |
| "price": product.price.description, | |
| "subscriptionGroupIdentifier": product.subscriptionGroupIdentifier as Any, | |
| "subscriptionPeriod": product.subscriptionPeriod.map { getMapFrom($0) } as Any, | |
| "priceLocale": getMapFrom(product.priceLocale), | |
| ] | |
| } | |
| public static func getMapFrom(_ product: SKProduct) -> [String: Any] { | |
| return [ | |
| "discounts": getMapArrayFrom(product.discounts), | |
| "introductoryPrice": product.introductoryPrice.map { getMapFrom($0) } ?? NSNull(), | |
| "localizedDescription": product.localizedDescription, | |
| "localizedTitle": product.localizedTitle, | |
| "productIdentifier": product.productIdentifier, | |
| "price": product.price.description, | |
| "subscriptionGroupIdentifier": product.subscriptionGroupIdentifier ?? NSNull(), | |
| "subscriptionPeriod": product.subscriptionPeriod.map { getMapFrom($0) } ?? NSNull(), | |
| "priceLocale": getMapFrom(product.priceLocale), | |
| ] | |
| } |
There was a problem hiding this comment.
Applied in aed96be — thanks. Worth noting for anyone reading later: Swift's Optional-in-Any bridging already converts a boxed nil to NSNull once the value crosses into Objective-C (verified locally: ["k": (nil as String?) as Any] as NSDictionary yields NSNull, not a crash), so the prior as Any wasn't actually crash-prone here. Making it explicit with ?? NSNull() is still clearer and matches what the original Objective-C implementation did explicitly (?: [NSNull null]), so applied as suggested.
| public static func getMapFrom(_ discount: SKProductDiscount) -> [String: Any] { | ||
| return [ | ||
| "identifier": discount.identifier as Any, | ||
| "numberOfPeriods": discount.numberOfPeriods, | ||
| "paymentMode": discount.paymentMode.rawValue, | ||
| "price": discount.price.description, | ||
| "subscriptionPeriod": getMapFrom(discount.subscriptionPeriod), | ||
| "type": discount.type.rawValue, | ||
| "priceLocale": getMapFrom(discount.priceLocale), | ||
| ] | ||
| } |
There was a problem hiding this comment.
Casting discount.identifier as Any can result in Optional<Any>.none being stored in the dictionary if the identifier is nil. This will cause a runtime crash when bridged to NSDictionary. Use ?? NSNull() to safely represent nil values.
| public static func getMapFrom(_ discount: SKProductDiscount) -> [String: Any] { | |
| return [ | |
| "identifier": discount.identifier as Any, | |
| "numberOfPeriods": discount.numberOfPeriods, | |
| "paymentMode": discount.paymentMode.rawValue, | |
| "price": discount.price.description, | |
| "subscriptionPeriod": getMapFrom(discount.subscriptionPeriod), | |
| "type": discount.type.rawValue, | |
| "priceLocale": getMapFrom(discount.priceLocale), | |
| ] | |
| } | |
| public static func getMapFrom(_ discount: SKProductDiscount) -> [String: Any] { | |
| return [ | |
| "identifier": discount.identifier ?? NSNull(), | |
| "numberOfPeriods": discount.numberOfPeriods, | |
| "paymentMode": discount.paymentMode.rawValue, | |
| "price": discount.price.description, | |
| "subscriptionPeriod": getMapFrom(discount.subscriptionPeriod), | |
| "type": discount.type.rawValue, | |
| "priceLocale": getMapFrom(discount.priceLocale), | |
| ] | |
| } |
| public static func getMapFrom(_ payment: SKPayment) -> [String: Any] { | ||
| return [ | ||
| "applicationUsername": payment.applicationUsername as Any, | ||
| "productIdentifier": payment.productIdentifier as Any, | ||
| "quantity": payment.quantity, | ||
| "requestData": payment.requestData.flatMap { String(data: $0, encoding: .utf8) } as Any, | ||
| "simulatesAskToBuyInSandbox": payment.simulatesAskToBuyInSandbox, | ||
| ] | ||
| } |
There was a problem hiding this comment.
Casting optional properties like applicationUsername and requestData to Any can result in Optional<Any>.none being stored in the dictionary, causing a runtime crash when bridged to NSDictionary. Use ?? NSNull() to safely represent nil values.
| public static func getMapFrom(_ payment: SKPayment) -> [String: Any] { | |
| return [ | |
| "applicationUsername": payment.applicationUsername as Any, | |
| "productIdentifier": payment.productIdentifier as Any, | |
| "quantity": payment.quantity, | |
| "requestData": payment.requestData.flatMap { String(data: $0, encoding: .utf8) } as Any, | |
| "simulatesAskToBuyInSandbox": payment.simulatesAskToBuyInSandbox, | |
| ] | |
| } | |
| public static func getMapFrom(_ payment: SKPayment) -> [String: Any] { | |
| return [ | |
| "applicationUsername": payment.applicationUsername ?? NSNull(), | |
| "productIdentifier": payment.productIdentifier, | |
| "quantity": payment.quantity, | |
| "requestData": payment.requestData.flatMap { String(data: $0, encoding: .utf8) } ?? NSNull(), | |
| "simulatesAskToBuyInSandbox": payment.simulatesAskToBuyInSandbox, | |
| ] | |
| } |
There was a problem hiding this comment.
Applied in aed96be, thanks. (Also dropped the now-redundant as Any on productIdentifier, which is non-optional on SKPayment.)
| public static func getMapFrom(_ locale: Locale) -> [String: Any] { | ||
| let nsLocale = locale as NSLocale | ||
| return [ | ||
| "currencySymbol": nsLocale.object(forKey: .currencySymbol) as Any, | ||
| "currencyCode": nsLocale.object(forKey: .currencyCode) as Any, | ||
| "countryCode": nsLocale.object(forKey: .countryCode) as Any, | ||
| ] | ||
| } |
There was a problem hiding this comment.
Casting the results of nsLocale.object(forKey:) directly to Any can result in Optional<Any>.none being stored in the dictionary, causing a runtime crash when bridged to NSDictionary. Use ?? NSNull() to safely represent nil values.
| public static func getMapFrom(_ locale: Locale) -> [String: Any] { | |
| let nsLocale = locale as NSLocale | |
| return [ | |
| "currencySymbol": nsLocale.object(forKey: .currencySymbol) as Any, | |
| "currencyCode": nsLocale.object(forKey: .currencyCode) as Any, | |
| "countryCode": nsLocale.object(forKey: .countryCode) as Any, | |
| ] | |
| } | |
| public static func getMapFrom(_ locale: Locale) -> [String: Any] { | |
| let nsLocale = locale as NSLocale | |
| return [ | |
| "currencySymbol": nsLocale.object(forKey: .currencySymbol) ?? NSNull(), | |
| "currencyCode": nsLocale.object(forKey: .currencyCode) ?? NSNull(), | |
| "countryCode": nsLocale.object(forKey: .countryCode) ?? NSNull(), | |
| ] | |
| } |
| public static func getMapFrom(_ transaction: SKPaymentTransaction) -> [String: Any] { | ||
| return [ | ||
| "error": transaction.error.map { getMapFrom($0 as NSError) } as Any, | ||
| "payment": (transaction.value(forKey: "payment") as? SKPayment).map { getMapFrom($0) } as Any, | ||
| "originalTransaction": transaction.original.map { getMapFrom($0) } as Any, | ||
| "transactionTimeStamp": transaction.transactionDate?.timeIntervalSince1970 as Any, | ||
| "transactionIdentifier": transaction.transactionIdentifier as Any, | ||
| "transactionState": transaction.transactionState.rawValue, | ||
| ] | ||
| } |
There was a problem hiding this comment.
Casting optional properties of SKPaymentTransaction to Any can result in Optional<Any>.none being stored in the dictionary, causing a runtime crash when bridged to NSDictionary. Additionally, use the standard originalTransaction property instead of original to avoid relying on custom extensions.
| public static func getMapFrom(_ transaction: SKPaymentTransaction) -> [String: Any] { | |
| return [ | |
| "error": transaction.error.map { getMapFrom($0 as NSError) } as Any, | |
| "payment": (transaction.value(forKey: "payment") as? SKPayment).map { getMapFrom($0) } as Any, | |
| "originalTransaction": transaction.original.map { getMapFrom($0) } as Any, | |
| "transactionTimeStamp": transaction.transactionDate?.timeIntervalSince1970 as Any, | |
| "transactionIdentifier": transaction.transactionIdentifier as Any, | |
| "transactionState": transaction.transactionState.rawValue, | |
| ] | |
| } | |
| public static func getMapFrom(_ transaction: SKPaymentTransaction) -> [String: Any] { | |
| return [ | |
| "error": transaction.error.map { getMapFrom($0 as NSError) } ?? NSNull(), | |
| "payment": (transaction.value(forKey: "payment") as? SKPayment).map { getMapFrom($0) } ?? NSNull(), | |
| "originalTransaction": transaction.originalTransaction.map { getMapFrom($0) } ?? NSNull(), | |
| "transactionTimeStamp": transaction.transactionDate?.timeIntervalSince1970 ?? NSNull(), | |
| "transactionIdentifier": transaction.transactionIdentifier ?? NSNull(), | |
| "transactionState": transaction.transactionState.rawValue, | |
| ] | |
| } |
There was a problem hiding this comment.
Applied the ?? NSNull() part in aed96be, thanks. One correction to the suggested diff, though: transaction.originalTransaction doesn't actually compile here — SKPaymentTransaction's Swift-imported name for that property is .original (the Clang importer renamed it back in Swift 3 to avoid the redundant type-name suffix; .originalTransaction fails with "has been renamed to 'original'"). It's not a custom extension, so I kept .original, which is correct and pre-existing.
| originalTransaction: transaction.original.flatMap { | ||
| convertTransaction(toPigeon: $0) | ||
| }, |
There was a problem hiding this comment.
Use the standard originalTransaction property instead of original to avoid relying on custom extensions.
| originalTransaction: transaction.original.flatMap { | |
| convertTransaction(toPigeon: $0) | |
| }, | |
| originalTransaction: transaction.originalTransaction.flatMap { | |
| convertTransaction(toPigeon: $0) | |
| }, |
There was a problem hiding this comment.
Not applying this one — transaction.originalTransaction doesn't compile. SKPaymentTransaction's Swift-imported name for this property is .original (renamed by the Clang importer since Swift 3; confirmed locally that .originalTransaction errors with "has been renamed to 'original'"). It isn't a custom extension, so the existing .original usage here is correct as-is.
…nary subscript Addresses gemini-code-assist review feedback: makes the nil-to-NSNull conversion explicit (matching the original Objective-C implementation's behavior) instead of relying on implicit Optional-in-Any bridging, and uses the default-value dictionary subscript in FIATransactionCache instead of a manual force-unwrap.
|
Thanks for the correction — you're right, and I appreciate the pointer. I dug in and confirmed Adopting it here is more than a one-line annotation, though.
Given that scope, would you rather this PR take on the pigeon-output switch as well, or land the ObjC→Swift core migration as currently scoped and follow up separately with the Swift-codegen switch for the Pigeon messages? Happy to do either — just want to keep this PR reviewable if a follow-up is preferred. |
I'll defer to the iOS reviewers on that, I just wanted to make sure it was understood that the final state of the overall migration should not include Obj-C Pigeon generation. |
2 participants
Ports
FIAObjectTranslator,FIAPReceiptManager,FIAPRequestHandler,FIAPPaymentQueueDelegate,FIATransactionCache, and their protocol shims (FLTPaymentQueueProtocol,FLTTransactionCacheProtocol,FLTMethodChannelProtocol,FLTRequestHandlerProtocol,FLTPaymentQueueHandlerProtocol) from Objective-C to Swift, completing the migration already started by the existingFIAPaymentQueueHandler.swift. No functional changes.Pigeon-generated
messages.g.m/.hintentionally stay Objective-C: Pigeon's Swift codegen represents data classes asstructs, which can't expressSKPaymentTransactionMessage's self-referentialoriginalTransactionfield (a struct can't recursively contain itself). The Swift sources import the (now much smaller)in_app_purchase_storekit_objctarget solely for these generated Pigeon types.Also ports the package's Objective-C test doubles (
Stubs.h/Stubs.m, ~850 lines) to Swift (Stubs.swift), since in this repo's SPM-based plugin setup theRunnerTeststarget has no way to@import/#importthe plugin's new Swift-only declarations from Objective-C (confirmed via several failed approaches: package product dependencies, generated-Swift.hheaders). Folded in the previously-duplicatedInAppPurchasePluginStub(ios/RunnerTests/SwiftStubs.swiftandmacos/RunnerTests/Stubs.swift, which held identical content) into the same shared file.Fixed a handful of real bugs surfaced while porting the ObjC test stubs to Swift's stricter type system:
NSNullvalues passed viasetValue(_:forKey:)intoNSString-typed properties (crashes when Swift bridges them), a couple of missingrequired init()overrides, an unset implicitly-unwrapped-optional test property, and one test assertion comparing two Pigeon-generated ObjC objects by identity instead of by field (they don't implementisEqual:). These were bugs in the test doubles, not the production code; the existing 76 test cases (unchanged) now pass again against the ported code.Part of #102679.
Pre-Review Checklist
[shared_preferences]0.4.11+2).///). — not applicable, no public API surface changed.Test plan
dart run script/tool/bin/flutter_plugin_tools.dart native-test --macos --packages in_app_purchase_storekit --no-integration— 76/76 tests passdart run script/tool/bin/flutter_plugin_tools.dart analyze --packages in_app_purchase_storekitdart run script/tool/bin/flutter_plugin_tools.dart validate --packages in_app_purchase_storekitdart run script/tool/bin/flutter_plugin_tools.dart format --fail-on-change --no-clang-format --packages in_app_purchase_storekitflutter build ios --config-onlyfailure against an untouched, unrelated plugin). iOS shares the exact same Swift source as macOS, which is fully green.Note: The Flutter team is currently trialing Gemini Code Assist for GitHub. Comments from
gemini-code-assist[bot]aren't authoritative Flutter-team feedback; I'll wait for a human reviewer's guidance on which automated comments (if any) should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩