[in_app_purchase_storekit] migrate main plugin class to swift in preperation to storekit 2#6561
Conversation
packages/in_app_purchase/in_app_purchase_storekit/darwin/Classes/in_app_purchase_storekit.h
Show resolved
Hide resolved
| List<String> productIdentifiers); | ||
|
|
||
| void finishTransaction(Map<String, String?> finishMap); | ||
| void finishTransaction(Map<String, Object?> finishMap); |
There was a problem hiding this comment.
Even though the intended type here should be a nullable String, when pigeon generates obj C code, it is generated as type NSString. NSString is nullable in objective c, but when Xcode generates the corresponding swift method signature, it converts into a String, not a String?. NSString also cannot be annoted with _Nullable as it is a generic.
There was a problem hiding this comment.
im confused. why is NSString generic?
There was a problem hiding this comment.
Sorry, its not generic, but you can't specify a type inside a NSDictionary to be nullable. (i dont think)
Specifically here -
What I really want is for this line in swift
To be
finishMap: [String: String?]. But the automatic conversion turns all NSStrings as String and not String?. And because I can't specify the NSString as extra nullable, I have to make it Object? (in pigeon) so it gets generated into id in objc, which then will turn into Any, and thus allow for nullabilityNot sure if that makes sense lol
There was a problem hiding this comment.
Using Any to represent an optional can be error-prone.
Why do you want [String:String?]? Is there any issue with using [String:String]?
There was a problem hiding this comment.
Not sure how pendingTransactions is related to the dictionary type. Can you explain how using [String:String] type would fail to work here?
There was a problem hiding this comment.
Yea I meant the comments
Im not understanding the question i think? finishMap should be [String:String?] because finishMap["transactionIdentifier"] should be allowed to be null.
There was a problem hiding this comment.
If it's just [String:String] then finishMap["transactionIdentifier"] wouldn't be allowed to be null, right?
But if finishMap["transactionIdentifier"] is null and a transaction in pendingTransactions also has a null transactionIdentifier then that would also be a valid transaction. If we had [String:String] then we couldn't check that?
There was a problem hiding this comment.
You can see on the dart side that the finishMap arg it gets called with has [String:String?] as well - I genuinely do not think it would be correct to make it [String:String]. Once this gets converted to using Swift pigeon, I can change it from String:Any to String:String?, so this should be temporary
There was a problem hiding this comment.
finishMap should be [String:String?] because finishMap["transactionIdentifier"] should be allowed to be null.
If it's just [String:String] then finishMap["transactionIdentifier"] wouldn't be allowed to be null, right?
Not really. dict[key] always returns optional. If type of finishMap is [String:String], then the type of finishMap["transactionIdentifier"] would be String?.
If you use [String:String?] as the type, then finishMap["transactionIdentifier"] would be String?? (double optional).
|
|
||
| @end | ||
|
|
||
| @interface InAppPurchasePluginStub () |
There was a problem hiding this comment.
This plugin stub needed to be converted to swift as you cannot subclass a swift class in objective C.
|
Could you run formatter first so it's easier for review? |
hellohuanlin
left a comment
There was a problem hiding this comment.
I briefly looked at the code and did not look into the details. I have to stop early as it's getting long, and there are enough feedback already.
packages/in_app_purchase/in_app_purchase_storekit/darwin/Classes/InAppPurchasePlugin.swift
Outdated
Show resolved
Hide resolved
packages/in_app_purchase/in_app_purchase_storekit/darwin/Classes/InAppPurchasePlugin.swift
Outdated
Show resolved
Hide resolved
packages/in_app_purchase/in_app_purchase_storekit/darwin/Classes/InAppPurchasePlugin.swift
Outdated
Show resolved
Hide resolved
packages/in_app_purchase/in_app_purchase_storekit/darwin/Classes/InAppPurchasePlugin.swift
Show resolved
Hide resolved
packages/in_app_purchase/in_app_purchase_storekit/darwin/Classes/InAppPurchasePlugin.swift
Outdated
Show resolved
Hide resolved
|
|
||
| public func handleTransactionsRemoved(_ transactions: [SKPaymentTransaction]) { | ||
| var maps: [[AnyHashable: Any]] = [] | ||
| for transaction in transactions { |
| return value is NSNull ? nil : value | ||
| } | ||
|
|
||
| func getRefreshReceiptRequest(properties: [String: Any]?) -> SKReceiptRefreshRequest { |
There was a problem hiding this comment.
are these helpers only used inside this file? should use private if that's the case.
There was a problem hiding this comment.
these need to be not private so they can be overriden for testing purposes
Like here:
packages/in_app_purchase/in_app_purchase_storekit/darwin/Classes/in_app_purchase_storekit.h
Show resolved
Hide resolved
| // | ||
|
|
||
| #import "Stubs.h" | ||
| #import "Stubs.m" |
There was a problem hiding this comment.
why is it importing the .m file?
| List<String> productIdentifiers); | ||
|
|
||
| void finishTransaction(Map<String, String?> finishMap); | ||
| void finishTransaction(Map<String, Object?> finishMap); |
There was a problem hiding this comment.
im confused. why is NSString generic?
I believe the From the original commit of the project: |
| MARKETING_VERSION = 1.0; | ||
| MTL_ENABLE_DEBUG_INFO = INCLUDE_SOURCE; | ||
| MTL_FAST_MATH = YES; | ||
| OTHER_SWIFT_FLAGS = "$(inherited) -D COCOAPODS -Xcc -fmodule-map-file=\"${PODS_ROOT}/Headers/Public/in_app_purchase_storekit/in_app_purchase_storekit.modulemap\" -Xcc -fmodule-map-file=\"${PODS_ROOT}/Headers/Public/integration_test/integration_test.modulemap\" -Xcc -fmodule-map-file=\"${PODS_ROOT}/Headers/Public/shared_preferences_macos/shared_preferences_macos.modulemap\""; |
There was a problem hiding this comment.
There are two more of these I believe, one for each configuration.
packages/in_app_purchase/in_app_purchase_storekit/darwin/Classes/InAppPurchasePlugin.swift
Outdated
Show resolved
Hide resolved
packages/in_app_purchase/in_app_purchase_storekit/darwin/Classes/InAppPurchasePlugin.swift
Outdated
Show resolved
Hide resolved
packages/in_app_purchase/in_app_purchase_storekit/darwin/Classes/InAppPurchasePlugin.swift
Show resolved
Hide resolved
packages/in_app_purchase/in_app_purchase_storekit/darwin/Classes/InAppPurchasePlugin.swift
Outdated
Show resolved
Hide resolved
packages/in_app_purchase/in_app_purchase_storekit/darwin/Classes/InAppPurchasePlugin.swift
Outdated
Show resolved
Hide resolved
packages/in_app_purchase/in_app_purchase_storekit/darwin/Classes/InAppPurchasePlugin.swift
Show resolved
Hide resolved
packages/in_app_purchase/in_app_purchase_storekit/darwin/Classes/InAppPurchasePlugin.swift
Show resolved
Hide resolved
packages/in_app_purchase/in_app_purchase_storekit/darwin/Classes/InAppPurchasePlugin.swift
Outdated
Show resolved
Hide resolved
packages/in_app_purchase/in_app_purchase_storekit/darwin/Classes/InAppPurchasePlugin.swift
Outdated
Show resolved
Hide resolved
packages/in_app_purchase/in_app_purchase_storekit/darwin/Classes/InAppPurchasePlugin.swift
Outdated
Show resolved
Hide resolved
packages/in_app_purchase/in_app_purchase_storekit/darwin/Classes/InAppPurchasePlugin.swift
Outdated
Show resolved
Hide resolved
packages/in_app_purchase/in_app_purchase_storekit/darwin/Classes/InAppPurchasePlugin.swift
Outdated
Show resolved
Hide resolved
packages/in_app_purchase/in_app_purchase_storekit/darwin/Classes/InAppPurchasePlugin.swift
Show resolved
Hide resolved
packages/in_app_purchase/in_app_purchase_storekit/darwin/Classes/InAppPurchasePlugin.swift
Outdated
Show resolved
Hide resolved
packages/in_app_purchase/in_app_purchase_storekit/darwin/Classes/InAppPurchasePlugin.swift
Outdated
Show resolved
Hide resolved
packages/in_app_purchase/in_app_purchase_storekit/darwin/Classes/InAppPurchasePlugin.swift
Outdated
Show resolved
Hide resolved
packages/in_app_purchase/in_app_purchase_storekit/darwin/Classes/InAppPurchasePlugin.swift
Outdated
Show resolved
Hide resolved
packages/in_app_purchase/in_app_purchase_storekit/darwin/Classes/InAppPurchasePlugin.swift
Outdated
Show resolved
Hide resolved
packages/in_app_purchase/in_app_purchase_storekit/darwin/Classes/InAppPurchasePlugin.swift
Outdated
Show resolved
Hide resolved
packages/in_app_purchase/in_app_purchase_storekit/darwin/Classes/InAppPurchasePlugin.swift
Outdated
Show resolved
Hide resolved
packages/in_app_purchase/in_app_purchase_storekit/darwin/Classes/InAppPurchasePlugin.swift
Show resolved
Hide resolved
packages/in_app_purchase/in_app_purchase_storekit/darwin/Classes/InAppPurchasePlugin.swift
Outdated
Show resolved
Hide resolved
| List<String> productIdentifiers); | ||
|
|
||
| void finishTransaction(Map<String, String?> finishMap); | ||
| void finishTransaction(Map<String, Object?> finishMap); |
There was a problem hiding this comment.
Using Any to represent an optional can be error-prone.
Why do you want [String:String?]? Is there any issue with using [String:String]?
hellohuanlin
left a comment
There was a problem hiding this comment.
LGTM! Thanks for making all the changes! Please make sure to manually test out if the feature still works as expected.
… in preperation to storekit 2 (flutter/packages#6561)
… in preperation to storekit 2 (flutter/packages#6561)
flutter/packages@fd714bd...87a02e3 2024-05-15 engine-flutter-autoroll@skia.org Roll Flutter from d2da1b2 to 39651e8 (18 revisions) (flutter/packages#6738) 2024-05-15 stuartmorgan@google.com Update the repo for the 3.22 stable release (flutter/packages#6730) 2024-05-15 linxunfeng@yeah.net [webview_flutter_wkwebview] Fixes JSON.stringify() cannot serialize cyclic structures (flutter/packages#6274) 2024-05-14 louisehsu@google.com [in_app_purchase_storekit] migrate main plugin class to swift in preperation to storekit 2 (flutter/packages#6561) 2024-05-14 34871572+gmackall@users.noreply.github.com [image_picker_android] Refactor getting of paths from intent to single helper (flutter/packages#5009) 2024-05-14 engine-flutter-autoroll@skia.org Roll Flutter (stable) from 54e6646 to 5dcb86f (1402 revisions) (flutter/packages#6727) 2024-05-14 10687576+bparrishMines@users.noreply.github.com [webview_flutter_wkwebview] Skip `withWeakReferenceTo` integration test (flutter/packages#6731) 2024-05-14 engine-flutter-autoroll@skia.org Roll Flutter from 1255435 to d2da1b2 (26 revisions) (flutter/packages#6729) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC flutter-ecosystem@google.com,rmistry@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…eration to storekit 2 (flutter#6561) Part of flutter/flutter#119106 This PR migrates the main InAppPurchasePlugin.m class to swift. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [relevant style guides] and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use `dart format`.) - [x] I signed the [CLA]. - [x] The title of the PR starts with the name of the package surrounded by square brackets, e.g. `[shared_preferences]` - [x] I [linked to at least one issue that this PR fixes] in the description above. - [x] I updated `pubspec.yaml` with an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes]. - [x] I updated `CHANGELOG.md` to add a description of the change, [following repository CHANGELOG style]. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] All existing and new tests are passing.
Part of flutter/flutter#119106
This PR migrates the main InAppPurchasePlugin.m class to swift.
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes].CHANGELOG.mdto add a description of the change, [following repository CHANGELOG style].///).