[in_app_purchase] Mostly convert to Android Pigeon #6262
Conversation
… mocks in earlier conversions
…me incorrect error handling in previous conversions
…a to Dart, and remove tests that no longer made sense
Convert all new code to Pigeon as part of the merge.
stuartmorgan-g
changed the title
|
This should be ready to review. There's a lot of code, but I went ahead and did all the methods in one PR since having a hybrid of manual method channel code and Pigeon code was kind of weird, particularly in tests. Each method is its own commit though, so it's possible to review incrementally, and I could cut it up if preferred. (It might be useful to look at one incremental commit to see the pattern, and then review in bulk after that?) @reidbaker If you could give this a manual sanity test, that would be great, since I don't have a setup to be able to do actual end-to-end tests. |
| }, | ||
| )) ?? | ||
| <String, dynamic>{}); | ||
| // TODO(stuartmorgan): Investigate whether forceOkResponseCode is actually |
There was a problem hiding this comment.
@reidbaker @gmackall FYI, someone who can dig into the expected behavior should probably take a look at this ASAP. This is a Dart conversion of this code, but for the reasons explained in this comment, I'm pretty skeptical that the Java code was correct. I deliberately didn't change it here since this PR is supposed to be a functional no-op, but I did want to flag it.
There was a problem hiding this comment.
Keep the code you have as is for this pr.
| billingResult -> { | ||
| result.success(fromBillingResult(billingResult)); | ||
| }); | ||
| try { |
There was a problem hiding this comment.
I had to add all of these try/catch wrappers to all the *Async methods because it turns out the plugin is relying on them as part of its API 🙁. We have an integration test that checks that when we pass an empty product query, for instance, that we get back a platform exception (with a specific string, even though we don't control that string, so that could be fun someday).
Ideally we should be preventing invalid cases explicitly instead, but that would change behavior. These FlutterErrors match the generic error made by the method channel code in the engine, so it's doing the same thing, it's just more explicit now (which means more boilerplate than pre-Pigeon, but also that we can see the anti-pattern—and the need to fix it—more clearly).
There was a problem hiding this comment.
Partial review. Stopped at https://github.com/flutter/packages/pull/6262/files#r1521913336
| if (billingClientError(result)) { | ||
| public void showAlternativeBillingOnlyInformationDialog( | ||
| @NonNull Result<PlatformBillingResult> result) { | ||
| if (billingClient == null) { |
There was a problem hiding this comment.
I think this is a behavior change for "endConnection" It used to be safe to call multiple times aka on a null billing client and with this check is is no longer ok.
There was a problem hiding this comment.
nvm there is no longer a shared onMethodCall.
|
|
||
| assertEquals(billingResultMap.get("responseCode"), newBillingResult.getResponseCode()); | ||
| assertEquals(billingResultMap.get("debugMessage"), newBillingResult.getDebugMessage()); | ||
| assertEquals(platformResult.getResponseCode().longValue(), newBillingResult.getResponseCode()); |
There was a problem hiding this comment.
It seems odd that we would need to get a long value here are you sure it is needed?
There was a problem hiding this comment.
Where I have these in tests it's because the Java compiler was angry at me for trying to pass a Long and a long (or sometimes int) to assertEquals, which wants the same kind of object.
stuartmorgan-g
added
the
autosubmit
flutter/packages@83f3842...0e848fa 2024-04-03 [email protected] [in_app_purchase] Mostly convert to Android Pigeon (flutter/packages#6262) 2024-04-02 [email protected] [various] Remove all traces of the `_ambiguate` workaround (flutter/packages#6449) 2024-04-02 [email protected] Roll Flutter from 7fa932b to a418568 (5 revisions) (flutter/packages#6450) 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 [email protected],[email protected] 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
Follow-up to #6262, converting the nested data objects from the legacy JSON conversion to typed Pigeon objects. Deprecates all the JSON code in the wrapper objects, but does not remove it because, unfortunately, the wrapper objects were both platform data transfer objects *and* the public API surface, so the JSON serialization/deserialization code that was part of those objects for platform channel usage is part of the public API as well. At some point we can do a breaking change to clean up that code (flutter/flutter#146202). Fixes flutter/flutter#117910
Replaces manual method channels with Pigeon. This replaces all of the method calls in both direction with Pigeon calls, and converts all the top-level objects used for params and returns to Pigeon objects. However, because of the significant object graph, in order to somewhat limit the scope of this PR I made a cut at that layer in the object graph, with nested objects still using the existing JSON serialization. In a follow-up PR, those will be converted to typed Pigeon objects as well, completing the transition. Unfortunately a significant amount of JSON code can't be removed yet even though it's now unused by the plugin, because it's part of the API of the public wrappers, so clients may be using it. Once all the JSON code is unused, we could `@Deprecated` all of it, and then could do a follow-up breaking change later to remove it. Most of flutter/flutter#117910
Follow-up to flutter#6262, converting the nested data objects from the legacy JSON conversion to typed Pigeon objects. Deprecates all the JSON code in the wrapper objects, but does not remove it because, unfortunately, the wrapper objects were both platform data transfer objects *and* the public API surface, so the JSON serialization/deserialization code that was part of those objects for platform channel usage is part of the public API as well. At some point we can do a breaking change to clean up that code (flutter/flutter#146202). Fixes flutter/flutter#117910
Replaces manual method channels with Pigeon.
This replaces all of the method calls in both direction with Pigeon calls, and converts all the top-level objects used for params and returns to Pigeon objects. However, because of the significant object graph, in order to somewhat limit the scope of this PR I made a cut at that layer in the object graph, with nested objects still using the existing JSON serialization. In a follow-up PR, those will be converted to typed Pigeon objects as well, completing the transition.
Unfortunately a significant amount of JSON code can't be removed yet even though it's now unused by the plugin, because it's part of the API of the public wrappers, so clients may be using it. Once all the JSON code is unused, we could
@Deprecatedall of it, and then could do a follow-up breaking change later to remove it.Most of flutter/flutter#117910
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.///).