Skip to content
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

[Android] Fixed promise never returned from requestSubscription() #806

Merged
merged 1 commit into from
Oct 25, 2019

Conversation

ancyrweb
Copy link

Calling requestSubscription() on Android result in the purchase being made but the promise is never called. This is due to an exception being thrown when trying to resolve the promise, specifically method threw 'com.facebook.react.bridge.objectalreadyconsumedexception' exception because the WriteableMap is already consumed when sendEvent is called.
By making a copy before calling it, we get a fresh WriteableMap that we can send to our promise without error.

Copy link
Owner

@hyochan hyochan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's see how it works! 👍

@hyochan hyochan added the 🍗 enhancement New feature or request label Oct 25, 2019
@hyochan hyochan merged commit ada6e42 into hyochan:master Oct 25, 2019
@hyochan hyochan mentioned this pull request Oct 25, 2019
hyochan added a commit that referenced this pull request Oct 25, 2019
Copy link
Contributor

@mars-lan mars-lan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hyochan FYI

@@ -522,8 +522,8 @@ public void onPurchasesUpdated(BillingResult billingResult, @Nullable List<Purch
item.putBoolean("isAcknowledgedAndroid", purchase.isAcknowledged());
item.putInt("purchaseStateAndroid", purchase.getPurchaseState());

promiseItem = item.copy();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the copy method is only added in RN 0..61. Should probably bump up the major version and update doc to reflect that.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mars-lan Hey! Thank you for the detail. Um... I should have to think about it since I feel kind of reluctant to separate the major version. @rewieer How do you think?

Copy link
Author

@ancyrweb ancyrweb Oct 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The copy method basically calls merge, so we can create a public WriteableMap copy(WritableMap original) inside DoobooUtils.java.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rewieer Sounds great! Would you like to give a PR for your idea?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hyochan sorry for the long delay, I was about to do it but it seems someone did it before me

@ancyrweb ancyrweb deleted the fix/copy branch October 26, 2019 05:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍗 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants