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
Code refactor & more proper null safety update #301
Code refactor & more proper null safety update #301
Conversation
bohdan1krokhmaliuk
commented
Jul 22, 2021
•
edited
edited
- General refactor of android part, small refactor of ios part
- updated logic for some methods and fixed AndroidStudio warnings
- replaced JSONObject with HashMap(supported by MethodChannels) in androidInAppPurchasePlugin.java
- deprecated originalJsonAndroid and dataAndroid as it is always the same object as transactionReceipt
- refactored Dart part
@@ -1,3 +1,6 @@ | |||
## 5.5.0 |
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.
PR: NOT FINISHED YET
@@ -4,32 +4,28 @@ | |||
import android.content.Context; |
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.
@hyochan What is the status on amazon IAP? Is it a working feature? I tried to unify Amazon IAPs maps to be the same as Android apps, but it is better to kill amazon IAPs in case it is not a working feature and nobody uses 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.
@bohdan1krokhmaliuk Honestly I've not tried Amazon at all because I am not using it. Also, this was developed by the community by @Rockvole.
How about separating the two variants as done in dooboolab-community/react-native-iap#1336?
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.
No no, I was wondering cause it would make two more variables not nullable
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.
Hi, Sorry I cant help much, I have not done any Android java development since this Amazon IAP work (2018 I think). The Amazon store may become more important soon since it will be used for Android apps in Windows 11.
https://www.theverge.com/2021/6/24/22548428/microsoft-windows-11-android-apps-support-amazon-store
/// android specific | ||
'dataAndroid: $dataAndroid, ' | ||
'signatureAndroid: $signatureAndroid, ' | ||
'isAcknowledgedAndroid: $isAcknowledgedAndroid, ' | ||
'autoRenewingAndroid: $autoRenewingAndroid, ' | ||
'purchaseStateAndroid: $purchaseStateAndroid, ' | ||
'originalJsonAndroid: $originalJsonAndroid, ' | ||
|
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.
not needed fields removed as those are the copy of transactionReceipt
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.
Sure you can remove it!
NSString* introductoryPriceNumber = @""; | ||
NSString* introductoryPricePaymentMode = @""; | ||
NSString* introductoryPriceNumberOfPeriods = @""; | ||
NSString* introductoryPriceSubscriptionPeriod = @""; | ||
NSString* introductoryPriceNumber; | ||
NSString* introductoryPricePaymentMode; | ||
NSString* introductoryPriceNumberOfPeriods; | ||
NSString* introductoryPriceSubscriptionPeriod; |
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.
There should be no default values
@(transaction.transactionDate.timeIntervalSince1970 * 1000), @"transactionDate", | ||
@(lround(transaction.transactionDate.timeIntervalSince1970 * 1000)), @"transactionDate", |
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.
int on dart side
} else if (_platform.isIOS) { | ||
return 'no-ops in ios'; | ||
} | ||
throw PlatformException( | ||
code: _platform.operatingSystem, message: "platform not supported"); |
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.
Platform specific methods should throw exceptions on not supported platforms
Future get consumeAllItems async { | ||
Future get consumeAllItemsAndroid async { | ||
if (_platform.isAndroid) { |
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.
Platform specific methods should end with platform name
); | ||
|
||
return extractItems(json.encode(result)); | ||
return extractItems(result ?? []); |
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.
Around the whole platform operations with strings were replaced with operations with maps. So method channels are sending maps mostly and there is no need to make another unnecessary json string encode/decode
@hyochan Could you please test if there are no type errors with subscriptions and consumables IAPs for android and IOS? As I use just non-consumables and it works fine with them? |
Hm. Looks like we need others to test this out because I don't have any related example currently 😞 |
This PR is stale because it has been open 90 days with no activity. Leave a comment or this will be closed in 7 days |