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.

Sign up for GitHub

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

API improvements #482

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

API improvements #482

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
87aa469
Prepare added for automatically setuping wrapper
Legoless Jul 30, 2024
c28a0d7
Change to interface
Legoless Jul 30, 2024
953c310
Update UpdatedCustomerInfoListener.cs
Legoless Jul 30, 2024
d1fe006
Update Purchases.cs
Legoless Jul 30, 2024
dfea856
Update Purchases.cs
Legoless Jul 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 45 additions & 13 deletions RevenueCat/Scripts/Purchases.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,26 @@ public partial class Purchases : MonoBehaviour

[Tooltip("A subclass of Purchases.UpdatedCustomerInfoListener component.\n" +
"Use your custom subclass to define how to handle updated customer information.")]
public UpdatedCustomerInfoListener listener;
[SerializeField]
private UpdatedCustomerInfoListener listener;

// Cache custom listener, if not from class.
private IUpdatedCustomerInfoListener customListener;
public IUpdatedCustomerInfoListener Listener
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can hold on this change until the next major as well, and release the change below

Copy link
Author

@Legoless Legoless Aug 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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: Listener instead of listener. 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 listener and Listener in 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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!

Copy link
Author

Choose a reason for hiding this comment

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

I can also do the Add/RemoveListener if you like, but not sure which change you want me to revert? The change to private?

Copy link
Contributor

Choose a reason for hiding this comment

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

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 👍

Copy link
Author

Choose a reason for hiding this comment

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

Cool, I understand, I'll get on it this week.

{
get => listener != null ? listener : customListener;
set
{
if (value is UpdatedCustomerInfoListener listenerValue)
{
this.listener = listenerValue;
}
else
{
customListener = value;
}
}
}

[Tooltip("An optional boolean. Set this to true if you have your own IAP implementation " +
"and want to use only RevenueCat's backend.\nDefault is false.\n" +
Expand Down Expand Up @@ -111,22 +130,32 @@ public partial class Purchases : MonoBehaviour

private void Start()
{
#if UNITY_ANDROID && !UNITY_EDITOR
Prepare();
Copy link
Contributor

Choose a reason for hiding this comment

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

This is great! Makes a lot of sense, thanks for adding this!


if (useRuntimeSetup) return;

Configure(string.IsNullOrEmpty(appUserID) ? null : appUserID);
GetProducts(productIdentifiers, null);
}

private void Prepare()
{
if (_wrapper != null)
{
return;
}
#if UNITY_ANDROID && !UNITY_EDITOR
_wrapper = new PurchasesWrapperAndroid();
#elif UNITY_IPHONE && !UNITY_EDITOR
#elif UNITY_IPHONE && !UNITY_EDITOR
_wrapper = new PurchasesWrapperiOS();
#else
#else
_wrapper = new PurchasesWrapperNoop();
#endif
#endif

if (!string.IsNullOrEmpty(proxyURL))
{
_wrapper.SetProxyURL(proxyURL);
}

if (useRuntimeSetup) return;

Configure(string.IsNullOrEmpty(appUserID) ? null : appUserID);
GetProducts(productIdentifiers, null);
}

private void Configure(string newUserId)
Expand Down Expand Up @@ -191,6 +220,9 @@ public void Setup(PurchasesConfiguration purchasesConfiguration)
///
public void Configure(PurchasesConfiguration purchasesConfiguration)
{
// Ensure wrapper is inited.
Prepare();

var dangerousSettings = purchasesConfiguration.DangerousSettings.Serialize().ToString();
_wrapper.Setup(gameObject.name, purchasesConfiguration.ApiKey, purchasesConfiguration.AppUserId,
purchasesConfiguration.ObserverMode, purchasesConfiguration.UsesStoreKit2IfAvailable, purchasesConfiguration.UserDefaultsSuiteName,
Expand Down Expand Up @@ -1240,18 +1272,18 @@ private void _receiveCustomerInfo(string customerInfoJson)
{
Debug.Log("_receiveCustomerInfo " + customerInfoJson);

if (listener == null) return;
if (Listener == null) return;

var response = JSON.Parse(customerInfoJson);
if (response["customerInfo"] == null) return;
var info = new CustomerInfo(response["customerInfo"]);
listener.CustomerInfoReceived(info);
Listener.CustomerInfoReceived(info);
}

// ReSharper disable once UnusedMember.Local
private void _handleLog(string logDetailsJson)
{
if (listener == null) return;
if (Listener == null) return;

var response = JSON.Parse(logDetailsJson);
var logLevelInResponse = response["logLevel"];
Expand Down
9 changes: 7 additions & 2 deletions RevenueCat/Scripts/UpdatedCustomerInfoListener.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,13 @@

public partial class Purchases
{
public abstract class UpdatedCustomerInfoListener : MonoBehaviour
public interface IUpdatedCustomerInfoListener
{
public void CustomerInfoReceived(CustomerInfo customerInfo);
}

public abstract class UpdatedCustomerInfoListener : MonoBehaviour, IUpdatedCustomerInfoListener
{
public abstract void CustomerInfoReceived(CustomerInfo customerInfo);
}
}
}