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

Fix for missing async await. #295

Merged
merged 2 commits into from
Oct 23, 2018
Merged

Fix for missing async await. #295

merged 2 commits into from
Oct 23, 2018

Conversation

JJMoon
Copy link
Contributor

@JJMoon JJMoon commented Oct 23, 2018

Add missing async, await notation.
이거 안드로이드는 괜찮은 지 함 보셔요..

@JJMoon JJMoon requested a review from hyochan October 23, 2018 09:09
@hyochan hyochan merged commit f3d4626 into master Oct 23, 2018
@hyochan
Copy link
Member

hyochan commented Oct 23, 2018

@JJMoon Can you read this? I don't quite get how this process resolve #279.

@hyochan
Copy link
Member

hyochan commented Oct 24, 2018

@lwansbrough Hi. I need your help on this PR. Do you think we need to call another async and await to ensure the finishes of the function? I feel like it is quite redundant. #279 was resolved with doing this fixes but I am still not sure why we need this. Or could be a better way.

@LinusU
Copy link
Member

LinusU commented Dec 17, 2018

This removes the return value from these functions, there was nothing wrong with them before.

Could this please be reverted?

@hyochan
Copy link
Member

hyochan commented Dec 17, 2018

@LinusU I felt this is quite weird too. However, @JJMoon swear to god that this should be done to fix #279. I've just looked into this one and feels like this wasn't the solution and it was just fragile linking problem which we should just unlink and link again. What do you think @LinusU @JJMoon ?

@LinusU
Copy link
Member

LinusU commented Dec 17, 2018

It's possible that Babel is rewriting the async function in some way that makes the Promise resolve later in the microtask scheduling queue than before, but I don't see how that could possibly affect anything 🤔

@hyochan
Copy link
Member

hyochan commented Dec 17, 2018

@LinusU Ok. I'll try to revert this tomorrow if I don't receive any other feedback on this. Especially from @JJMoon.

@hyochan
Copy link
Member

hyochan commented Dec 18, 2018

@LinusU Wouldn't this be fine tough?

async () => RNIapIos.buyProduct(sku),

I've just looked in the code and saw that I've already gave some updates.

@LinusU
Copy link
Member

LinusU commented Dec 18, 2018

The async there won't do anything, I would recommend keeping it as () => RNIapIos.buyProduct(sku)

@hyochan
Copy link
Member

hyochan commented Dec 18, 2018

@LinusU Yes. This is just another Promise resolver. Ok I'll change this now then.

hyochan added a commit that referenced this pull request Dec 18, 2018
Revert all the async as discussed in #295.
@hyochan
Copy link
Member

hyochan commented Dec 18, 2018

@LinusU Done it!. @JJMoon Please check if you have any opinion on this release because you've told me to make changes to include async earlier. Thanks.

@JJMoon
Copy link
Contributor Author

JJMoon commented Dec 19, 2018

@hyochan It looks my PR is merged. It's OK. Thanks

@hyochan hyochan deleted the buysubscription branch December 19, 2018 05:19
@hyochan
Copy link
Member

hyochan commented Dec 19, 2018

@JJMoon That wasn't what I was saying. The changes you've made which you swear to god to make all the functions to async, we thought this should be reverted. Please read the previous conversations. Thanks.

@hyochan hyochan added the 🛠 bugfix All kinds of bug fixes label Dec 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🛠 bugfix All kinds of bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants