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 subscription upgrade/downgrade #188

Merged
merged 4 commits into from
Jun 11, 2018
Merged

Android subscription upgrade/downgrade #188

merged 4 commits into from
Jun 11, 2018

Conversation

vshab
Copy link
Contributor

@vshab vshab commented Jun 10, 2018

Hello!

Add optional oldSku parameter for buySubscription on Android to support subscription upgrade/downgrade.

Fix #185

@hyochan
Copy link
Owner

hyochan commented Jun 10, 2018

@vshab Won't this be better to create another method for this one? Because, we need some migration guide which can break in some developer's env.

@vshab
Copy link
Contributor Author

vshab commented Jun 10, 2018

@dooboolab this change shouldn't change anything for exciting apps like in 99% I would say. The oldSku parameter is optional, so existing code should just work. I think that something is possible if somebody for some reason passing extra parameter in buySubscription method which doesn't make any sense now :)
I just didn't want to create more methods which make sense only for only one platform in some specific case because it leads to more sophisticated API.
However if you think that this should be done as an another method I can do it, no problem :)

@hyochan
Copy link
Owner

hyochan commented Jun 11, 2018

@vshab Thank you for your confirm. I am confused. I thought methods with different number of parameters are different. Below are two different methods in js isn't it? Correct me if I am wrong.

helloMethod1(yo1, yo2) => {
}
helloMethod1(yo1) => {
}

Also, to be optional in ts, you should put ? in index.d.ts

export function buySubscription(sku: string, oldSku: string) : Promise<SubscriptionPurchase>;

to

export function buySubscription(sku: string, oldSku?: string) : Promise<SubscriptionPurchase>;

@vshab
Copy link
Contributor Author

vshab commented Jun 11, 2018

@dooboolab

Below are two different methods in js isn't it? Correct me if I am wrong.

Not exactly. When calling you can pass as many arguments to function as you want, the difference is how to access them inside of a function. Lest look at two functions for example:

function foo(a) {
  console.log(a, arguments[1]);
}

function bar(b, c) {
  console.log(b);
}

We can call first one with two arguments:

foo(1, 2);

and get 1 2

And call second one with one argument:

bar(3);

and get 1

By default function arguments are set to undefined
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Default_parameters

In JavaScript, parameters of functions default to undefined.

So basically in JS function can accept any number of arguments and naming them in definition only helps accessing. If there are less arguments in call than in definition then unspecified arguments will be undefined. That's what happening when old code will call buySubscription - oldSku by default would be undefined and everything should work as before. However new code can set oldSku and get new functionality.

Also, to be optional in ts, you should put ? in index.d.ts

You are right here! I'm not very good in TypeScript :(

@vshab
Copy link
Contributor Author

vshab commented Jun 11, 2018

@dooboolab i updated TypeScript definition.

@hyochan
Copy link
Owner

hyochan commented Jun 11, 2018

@vshab Thank you for your specific explanation. I was confused with the java. Have you tested this with non-optional and optional function call? Because, I think public void buyItemByType(String type, String sku, String oldSku, Promise promise) and public void buyItemByType(String type, String sku, Promise promise) will be recognized as different method in java.

@vshab
Copy link
Contributor Author

vshab commented Jun 11, 2018

@dooboolab yep, I've tested the code.
Here is the call without second argument:
2018-06-11 16 05 51

And here is with:
2018-06-11 16 05 45

@hyochan
Copy link
Owner

hyochan commented Jun 11, 2018

Ok. I will merge this and publish to newer version of npm which will be 1.1.4. Let's see what happens. Thank you for your hard work by the way!

@hyochan hyochan merged commit 17ac0de into hyochan:master Jun 11, 2018
@vshab
Copy link
Contributor Author

vshab commented Jun 11, 2018

@dooboolab 🎉 I hope we won't break anything :)
Thanks for quick responses!

hyochan added a commit that referenced this pull request Jun 12, 2018
@hyochan
Copy link
Owner

hyochan commented Jun 12, 2018

@vshab Thanks for your new feature. Also, there was a break in this PR so I quickly fixed this in react-native-iap@1.1.6. Java is not working like in js. It distinguishes method with number of parameters too. This is just to inform you if there can be any improvement with my fix. Thank you for your work again.

image

@vshab
Copy link
Contributor Author

vshab commented Jun 12, 2018

@dooboolab oh.. that's unfortunate. But good job fixing it! :)
I'll look if there anything else can be done.

@hyochan hyochan added 🤖 android Related to android 🎯 feature New feature labels Dec 20, 2018
marzuq-adebayo-dev added a commit to marzuq-adebayo-dev/React-Native-iap that referenced this pull request May 20, 2023
dev-arrow added a commit to dev-arrow/react-native-iap that referenced this pull request Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖 android Related to android 🎯 feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants