-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[in_app_purchase] Ensure the introductoryPriceMicros field is transported as a String. #4370
Conversation
I understand the desire to return a String to avoid a breaking change, but the |
/cc @cyanglaz, @stuartmorgan, how do you feel about the comment from @781flyingdutchman? I agree with him it would make the API a more consistent API, however it will still result in a breaking change since we would change the type of a public field. Even though this field was never populated correctly, developers could still access it. |
There's a difference between code that compiles but returned nothing at runtime and code that doesn't compile. It appears from blame that people have had four months, not two days, to write code that would fail to build if this changed. It's a breaking change. @mvanbeusekom This isn't forwarded through the cross-platform API, right; i.e., someone must directly use (We should still do a bugfix in a bugfix update first though, so that the last version people will get that's resolvable with existing dependencies isn't actively broken.) |
// Make sure the `introductoryPriceAmountMicros` field is returned as a `String` value as the | ||
// Dart side expects it as `String`. Changing the type in Dart would introduce a breaking change. | ||
info.put( | ||
"introductoryPriceAmountMicros", String.valueOf(detail.getIntroductoryPriceAmountMicros())); |
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.
Changing this on the Java side will make a better fix later harder to do. Can't we change the transport name, and have a String getter on the dart side that returns the underlying int value as a String?
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.
@stuartmorgan I understand changing to int would technically be a breaking change, but only for people who read and use the field - and that field would always have returned an empty string. I therefore expect the number of builds that break to be very very small.
Is a compromise to change the field to an int
and also add a getter introductoryPriceAmountMicrosAsString
that does the conversion so the very few people who read the original field and expect a string can simply change their field to the new getter?
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.
@stuartmorgan I understand changing to int would technically be a breaking change, but only for people who read and use the field
I'm well aware that it's only a breaking change for people who reference the field; please re-read my comment.
I therefore expect the number of builds that break to be very very small.
That may be true (or may not; you don't really know how many people did things like try to use it, find that it didn't seem to work for them, and added a fallback but left the code in place), but the definition of a non-breaking change in semantic versioning is not "I don't think it'll break many people".
Is a compromise to change the field to an
int
and also add a getterintroductoryPriceAmountMicrosAsString
that does the conversion so the very few people who read the original field and expect a string can simply change their field to the new getter?
No, because that's a breaking change.
There is no way to change the type of the existing getter without it being a breaking change; this is not a matter of discussion, it's an objective fact. And we're not going to make an obviously breaking change without adjusting the version accordingly, because we do not deliberately violate semantic versioning in our plugins.
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.
Got it - didn't mean to come across as pushing, was just trying to help. I have absolutely no skin in this game (I came across the original error, I don't use the field at all).
If I may make one comment, it would be that the impact of the original change (that caused the error) was quite severe, in that it rendered the entire package unusable for Android users. Ideally, this would have been caught by tests before bumping the version. I am saying this because I had the same thing happen with the wakelock package just a few days ago, and if you have a flutter app with many package dependencies then these bugs as a result of version updates are very disruptive and difficult to discover and trace back to the offending package. It leads me to consider locking packages to a specific version (instead of using the caret notation to benefit from upgrades), and that is not a good developer experience.
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.
If I may make one comment, it would be that the impact of the original change (that caused the error) was quite severe, in that it rendered the entire package unusable for Android users. Ideally, this would have been caught by tests before bumping the version.
Yes, it's not clear to me yet why that PR passed our automated tests (of which we have quite a few). That's something we'll be looking into.
I am saying this because I had the same thing happen with the wakelock package just a few days ago
That was a third-party package, and as far as I can tell from the nature of that bug, that plugin:
- has no automated testing of any kind, and
- was never launched on iOS after the change
We cannot control what third-party package authors do, but it was a very different scenario. That bug could not have passed our CI.
I would suggest the following (cooperating with what @stuartmorgan has commented):
Then open another PR, making a breaking change (major version bump). This PR removes the deprecated As for testing, I'm not aware that we have anything specific setup to make sure method channels work end to end (I might have missed something?). In iOS, we have StoreKit Tests, which is a UITests. It does cover the end to end case. However, StoreKit Test equivalent does not exist in Play Billing Library just yet. |
I didn't realize we didn't have integration tests for this plugin. In that case, we should look into setting up "native end to end" tests as discussed in the testing doc, where we could mock out the Play dependencies. |
(We should also seriously evaluate using Pigeon here in the medium term, so that we have less room to make type errors, if we don't have sufficient test coverage.) |
d4482c5
to
15bcf56
Compare
@stuartmorgan, @cyanglaz, I have updated the PR to introduce the |
@@ -1,6 +1,6 @@ | |||
## 0.1.4+7 |
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 was published; it shouldn't be removed from the changelog.
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.
You are right and fixed it.
...pp_purchase/in_app_purchase_android/lib/src/billing_client_wrappers/sku_details_wrapper.dart
Show resolved
Hide resolved
...n_app_purchase_android/test/billing_client_wrappers/sku_details_wrapper_deprecated_test.dart
Show resolved
Hide resolved
...n_app_purchase_android/test/billing_client_wrappers/sku_details_wrapper_deprecated_test.dart
Show resolved
Hide resolved
required this.introductoryPriceMicros, | ||
@Deprecated('Use `introductoryPriceAmountMicros` parameter instead') | ||
String introductoryPriceMicros = '', | ||
this.introductoryPriceAmountMicros = 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.
I think the better option here is to make this int introductoryPriceAmountMicros,
, and have the constructor body do:
introductoryPriceAmountMicros = introductoryPriceAmountMicros ?? int.tryParse(introductoryPriceMicros) ?? 0
Then you don't need all the complicated logic in the getter, or _introductoryPriceMicros
.
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 sure that will work as the introductoryPriceAmountMicros
parameter a not nullable and should always have a value. We could make it nullable but that means we need to later make it non-nullable again when we introduce the breaking change.
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.
We'll be changing it to required at that point presumably to match everything else, and it will already be a breaking change in general, so that doesn't seem like a significant issue. But it's not a big deal either way.
@Deprecated('Use `introductoryPriceAmountMicros` instead.') | ||
@JsonKey(ignore: true) | ||
String get introductoryPriceMicros => _introductoryPriceMicros.isEmpty | ||
? introductoryPriceAmountMicros != 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.
I didn't get why we needed the nested part. If introductoryPriceAmountMicros is 0, can't introductoryPriceMicros just be '0'?
Wouldn't the below work?
String get introductoryPriceMicros => _introductoryPriceMicros.isEmpty ?
introductoryPriceAmountMicros.toString() :
_introductoryPriceMicros;
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.
Yes it would work, but the original behavior (although wrong) was to return an empty string as default value. I tried to stick as close as possible to that using the nested condition.
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.
The original behavior always returns empty string because of the incorrect KV mapping, right?
If returning empty string is wrong, we don't need to worry about the wrong behavior. This PR suppose to "fix" 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.
Yes that makes sense. In that case the inner condition can indeed be removed.
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.
Done.
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.
LGTM, let's wait for @stuartmorgan's approval on the things he commented.
@@ -68,8 +72,15 @@ class SkuDetailsWrapper { | |||
final String introductoryPrice; | |||
|
|||
/// [introductoryPrice] in micro-units 990000 |
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.
Can we add a new comment saying something similar to the android doc:
/// Returns 0 if the SKU is not a subscription or doesn't have an introductory period.
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.
Done, good catch. I think that is a valuable addition.
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.
LGTM
This pull request is not suitable for automatic merging in its current state.
|
Introduced the `SkuDetailsWrapper.introductoryPriceAmountMicros` field of the correct type (`int`) and deprecated the `SkuDetailsWrapper.introductoryPriceMicros` field.
Added extra documentation explaining that the default return value for the `SkuDetailsWrapper.introductoryPriceAmountMicros` field will be 0 in case the product is not a subscription or doesn't have an introductory price associated with it.
67e696a
to
0fbf9e3
Compare
… transported as a String. (flutter/plugins#4370)
* master: [google_maps_flutter] Add Marker drag events (flutter#2838) [flutter_plugin_tools] Validate pubspec description (flutter#4396) Add file_selector to the repo list (flutter#4395) [in_app_purchase] Fix in_app_purchase_android/README.md (flutter#4363) [google_maps_flutter_web] Add Marker drag events (flutter#4385) [webview_flutter] Fixed todos in FlutterWebView.java (flutter#4394) Handle `PurchaseStatus.restored` correctly in example. (flutter#4393) Handle restored purchases in iOS example app (flutter#4392) [file_selector] Remove custom analysis options (flutter#4382) [flutter_plugin_tools] Check licenses in Kotlin (flutter#4373) Fixed _CastError when running example App (flutter#4390) [in_app_purchase] Ensure the introductoryPriceMicros field is transported as a String. (flutter#4370) Load navigation controls immediately. (flutter#4377) [camera] Fix IllegalStateException being thrown in Android implementation when switching activities. (flutter#4319) # Conflicts: # packages/webview_flutter/webview_flutter/CHANGELOG.md # packages/webview_flutter/webview_flutter_android/CHANGELOG.md
* master: (1126 commits) [webview_flutter] Adjust test URLs again (flutter#4407) [google_sign_in] Add serverAuthCode attribute to google_sign_in_platform_interface user data (flutter#4179) [camera] Add filter for unsupported cameras on Android (flutter#4418) [webview_flutter] Update webview platform interface with new methods for running JavaScript. (flutter#4401) [webview_flutter] Add zoomEnabled to webview flutter platform interface (flutter#4404) [ci] Remove obsolete Dockerfile (flutter#4405) Fix order-dependant platform interface tests (flutter#4406) [google_maps_flutter]: LatLng longitude loses precision in constructor #90574 (flutter#4374) [google_maps_flutter] Add Marker drag events (flutter#2838) [flutter_plugin_tools] Validate pubspec description (flutter#4396) Add file_selector to the repo list (flutter#4395) [in_app_purchase] Fix in_app_purchase_android/README.md (flutter#4363) [google_maps_flutter_web] Add Marker drag events (flutter#4385) [webview_flutter] Fixed todos in FlutterWebView.java (flutter#4394) Handle `PurchaseStatus.restored` correctly in example. (flutter#4393) Handle restored purchases in iOS example app (flutter#4392) [file_selector] Remove custom analysis options (flutter#4382) [flutter_plugin_tools] Check licenses in Kotlin (flutter#4373) Fixed _CastError when running example App (flutter#4390) [in_app_purchase] Ensure the introductoryPriceMicros field is transported as a String. (flutter#4370) ... # Conflicts: # packages/quick_actions/ios/Classes/FLTQuickActionsPlugin.m
…rted as a String. (flutter#4370)
…rted as a String. (flutter#4370)
In the current version the
introductoryPriceMicros
field is transported from JAVA to Dart as along
type. This is causing a runtime error on the Dart side as it is expected to be of typeString
(see comment here).This PR will resolve the problem by making sure the
introductoryPriceMicros
field is converted to aString
type before it is transported to Dart.Ideally we would change the Dart API to expect the field to be a
int
, however this would introduce a breaking change. Therefore I opted for the conversion fromlong
toString
on the JAVA side.Fixes the problem discussed here: #4364 (comment)
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
dart format
.)[shared_preferences]
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.