API improvements #482
API improvements #482
Conversation
There was a problem hiding this comment.
Hi @Legoless,
Really sorry for the delay, and thanks for your contribution! The changes here make a lot of sense. I just left a comment about a possible breaking change. What do you think?
Once we get that sorted I can get this merged
|
|
||
| // Cache custom listener, if not from class. | ||
| private IUpdatedCustomerInfoListener customListener; | ||
| public IUpdatedCustomerInfoListener Listener |
There was a problem hiding this comment.
I believe this means that devs setting the listener through code before might experience a breaking change.
- Before:
purchases.listener = CustomListener()
- After:
purchases.Listener = CustomListener()
I do think that most people would set the listener through the editor, considering it had to be a MonoBehavior... But I think we might need to make the listener public private still to avoid the breaking change, and encourage people to move to use Listener instead. Wdyt?
There was a problem hiding this comment.
Maybe we can hold on this change until the next major as well, and release the change below
There was a problem hiding this comment.
You're correct, this is breaking change in code. I decided for this approach for two reasons:
- It's very easy to search and replace in code:
Listenerinstead oflistener. Could leave some comments and hint maybe for those people that use it in code. It's unlikely people would be setting and resetting it a lot of times either. - It's very clear from API design perspective that the private field is meant for Editor only and you only allow setting it there. In other way you would have two exposed
listenerandListenerin the API, which could get confusing, if in fact you did want to set it in code as a new user.
My suggestion would be to keep the API clearer and rather make a major release (or however you think is best). If this breaking change is a problem, then this approach will make the API clearer in the future. But at the end it's your call how you want to have it, definitely both solutions work for our case. Maybe a better approach would be to have a List of those listeners instead and keep the original one public and just add like a AddListener/RemoveListener methods to manage the list.. Just thinking out loud.
Your call, let me know if you need me to change in the PR.
There was a problem hiding this comment.
Yeah I agree that it would be better to ship this in a major, rather than keeping both which would cause a very confusing API.
If you want to revert this change and leave the one below, we would be happy to get that in!
As for your proposal of having AddListener/RemoveListener`, I think that could work! We actually allow doing this in some other SDKs. But seems that could be worked on separately. Thanks for the suggestion!
There was a problem hiding this comment.
I can also do the Add/RemoveListener if you like, but not sure which change you want me to revert? The change to private?
There was a problem hiding this comment.
Sorry for not being clear! My suggestion is to:
- Revert the changes related to the listener (including the new interface)
- Keep the changes related to initialization of the SDK at different points of time.
Then merge the PR with just that second change, which shouldn't be a breaking change. And we can release that soon.
As for doing the Add/Remove listener, I think that works too! And it would be amazing if you can do it 👼. The only point is to avoid breaking changes (at least until we're ready to make another major)
Please let me know if you have questions or help with the listener approach! We also think that would be a good addition 👍
There was a problem hiding this comment.
Cool, I understand, I'll get on it this week.
|
|
||
| // Cache custom listener, if not from class. | ||
| private IUpdatedCustomerInfoListener customListener; | ||
| public IUpdatedCustomerInfoListener Listener |
There was a problem hiding this comment.
Maybe we can hold on this change until the next major as well, and release the change below
| @@ -111,22 +130,32 @@ public partial class Purchases : MonoBehaviour | |||
|
|
|||
| private void Start() | |||
| { | |||
| #if UNITY_ANDROID && !UNITY_EDITOR | |||
| Prepare(); | |||
There was a problem hiding this comment.
This is great! Makes a lot of sense, thanks for adding this!
This PR adds two API improvements:
Purchases Listener is currently an abstract
MonoBehaviour, which ties the behaviour to Unity components. This is not bad, but it does limit you, which is entirely not needed, if you mostly use the API from code. There's no other requirement that this should in fact be an abstract class (aside from the ability to set it in Editor). So a new interface is introduced instead and backwards support is added, so that theUpdatedCustomerInfoListenerimplements the interface. Field is now set as private and serialized, so it can remain set in Editor, but via codeListenerproperty should be used.The whole
Purchasesobject which is required for RevenueCat to operate initializes itself fairly late inStartfunction. This is a problem, if you need to do anything beforehand as we're booting our game up inAwakemethod. The script order is usually undefined and we've seen many differences in it between real devices and Editor, so sometimesPurchaseswould prepare itself for API type of configuration, sometimes we would be too early callingConfigureand crash withNullPointerExceptionon internal_wrapper. To get around this problem, we extracted initialization of wrapper to a separate function that only creates the instances once, but can be called many times. That wayConfigurewillPreparethe wrapper, before it calls to it'sSetupmethod.The issue number(s) or PR number(s) in the description if you are contributing in response to those.
No open issues right now for these improvements.
If applicable, unit tests.
No additional Unit Tests required.