Unity native/web interface for checking out

[sleroux]

Problem

Jeffrey and I spent some time today talking about how we want to handle the checkout integration into the Unity side of the project. Initially we were thinking of having the checkout logic live in Cart.cs but when thinking ahead to the web/Android implementations we some some short comings:

1. When the game developer checks out they pass in callback actions which would be stored on Cart.cs. This adds more responsibility to the Cart.cs class and adds more potentially platform-specific state.

2. The Cart.cs would contain both logic for iOS and Android checkout logics which lengthens the implementation and adds more sprinkling of #ifdefs.

Solution

To break up the concerns of the Cart and Checkout methods,

1. Extract checkout logic from Cart.cs and forward API calls to an implementor.
2. Split up the checkout implementation across platform lines.

To accomplish this, we define a new interface for handling checking out:

interface ICartCheckout {
    void StartWebCheckout(Action success, Action failure);
    bool CanShowNativePaySetup();
    void ShowNativePaySetup();
    void PrepareNativeCheckout();
    void CompleteNativeCheckout(Action success, CheckoutFailure failure);
}

This will allow us to define different implementations for the various platforms as well has having a single place to store state related to checking out:

// iOS
partial class IOSCartCheckout: ICartCheckout {
    // Platform specific plugin methods.
    [DllImport ("__internal")]
    private static extern void _StartWebCheckout

    // Storage for callbacks, etc.
    private Action success, failure;

    // Implementation of checkout methods that previously would have been in Cart.cs.
    void StartWebCheckout(Action success, Action failure) {}
        // Setup global game object with receiver
        this.success = success;
        this.failure = faiure;
        _StartWebCheckout(...);
    }
}

We can also extend this class to implement the different interfaces for the native -> Unity messages:

partial class IOSCartCheckout: ApplePayEventsReceiver {

}

partial class IOSCartCheckout: WebPayEventsReceiver {
    void DidCompleteWebCheckout(string result) {
        result ? success() : failure();
    }
}

With the implementations contained in the ICartCheckout objects, we can now create a platform-specific instance when the Cart is created:

// Checkout methods and implementation
class Cart  {
    private ICartCheckout cartCheckout;

    public Cart() {
        // Load in platform-specific implementor. Pass in cart for mutations if needed.
    #if UNITY_IOS
        cartCheckout = new IOSCartCheckout(this);
    #endif
    #if UNITY_ANDROID
        cartCheckout = new AndroidCartCheckout(this);
    #endif
    }

    // Game developer API which forwards to the implementor (no #ifdefs!).
    void StartWebCheckout(Action success, Action failure)  {
        cartCheckout.StartWebCheckout(success, failure);
    }
}

A nice side effect of splitting it up like this is we can mock out the checkout methods for easier unit testing.

[sleroux]

Not sure how I request feedback. Do I just type this message with your handles? 😄 @mikkoh @richardmonette @dengjeffrey

[richardmonette]

Overall I think this is a good direction.

I noticed in the interface you have CanShowNativePaySetup, would we have explicit checks on each type of action? I wonder if it would be better to have a general exception that we raise for parts of the interface which are not implemented on a particular platform. This could avoid having a mix of exception handling and explicit error checking.

[sleroux]

I noticed in the interface you have CanShowNativePaySetup, would we have explicit checks on each type of action? I wonder if it would be better to have a general exception that we raise for parts of the interface which are not implemented on a particular platform.

Just to clarify, do you see this method not being needed and baking in the check into the existing prepare/complete flow (ie. throw an 'unsupported native pay' error when we can't do native checkouts)? That sounds awesome since it would let the game developer to funnel the exceptional cases into the same code path.

[dengjeffrey]

Noticed that the interface is missing: bool CanCheckoutWithNativePay(); We need these checks so that the developer can display or hide buttons based on what is available. Some developer might want to separate the Pay with Apple Pay and Checkout buttons, hence why we need a check like CanCheckoutWithNativePay

[richardmonette]

Some developer might want to separate the Pay with Apple Pay and Checkout buttons, hence why we need a check like CanCheckoutWithNativePay

Good point, OK, so agreed, we do need those functions for checking functionality, in order to do things like customize UI.

[mikkoh]

Just want to be clear about this. What do you envision the public API looking like? Would Cart simply instantiate an IOSCartCheckout or AndroidCardCheckout or are you actually thinking that you'd pass a Cart to a CartCheckout?

At one point @JakeCataford and I were thinking of having something like new Checkout(cart). I shyd away from this at the time because I didn't see a benefit for the end user and it was hard to separate the state of a cart and checkout.

[sleroux]

The checkout API would be the same as Jeff discussed in https://github.com/Shopify/unity-buy-sdk/issues/65 where we add methods directly to the Cart. The Cart would instantiate a platform-specific implementation behind the scenes and forward the calls to it. The goal is to decouple the checkout out logic from the Cart since there's some state needs to be stored that muddies up the Cart implementation.

[sleroux]

I pushed up the start of the code here to take a 👀 at https://github.com/Shopify/unity-buy-sdk/pull/114

[JakeCataford]

What is a CartCheckout though? I'm worried that doubling down on joining these concepts is going to bite us later because we will want to implement some of the more specific features that a "checkout" or a "cart" can do, and end up making compromises because we cant properly map the api to our data model.

I like ICartCheckout in the sense that there is nothing wrong with having a common interface for these similar types... I would probably name it something else though that properly encapsulates "what" a checkout a cart have in common.

[sleroux]

I'm in the middle of tackling this from both the iOS and C# sharp side to make sure that things align well on our internal API and having an interface for CartCheckout doesn't have much meaning. Naming this something more specific to it's behaviour makes more sense.

I'm also finding that it might make more sense to break this up on the feature set (web/native) as well as platform.

What do you think about taking ICartCheckout and splitting it up into two better named interfaces named something like IWebPay and INativePay?

[JakeCataford]

Yea or even more javaesque could be IWebPayable lol... but thats pedantic... I think those pay interface names get the point across.

These could also be partially joined in a IPayable as well, but you have better context than me on that side of the project, so i'll defer to you on that one

[mikkoh]

@JakeCataford There's a piece of info I'm missing here. What are advantages to separating checkouts and carts?

[sleroux]

Had a discussion with @richardmonette and @dengjeffrey about both the API interface for Unity developers and our internal callbacks which builds on this.

For the Unity interface, we were thinking that instead of using callbacks/delegates to relay events that occur during the checkout, we could leverage the existing UnityEvent infrastructure to bubble these up in a way that is familiar with Unity devs. Taking the existing methods and moving them to this model would look something like this from a game dev perspective:

// Each of these would be UnityEvent properties. If we want we can add more events to this
// as we get feedback from developers as to what they want to be told from the SDK.
cart.onCheckoutSucceded.AddListener(...);
cart.onCheckoutFailed.AddListener(...);
cart.onCheckoutCancelled.AddListener(...);

cart.StartWebCheckout();
cart.StartNativeCheckout(merchantID);

Additionally we could have a static method on Cart named SupportsNativePay that the game dev can use to to determine if the platform is able to do Apple/Android Pay.

After some initial brainstorm we think we can use this same API regardless of platform and let the SDK handle the nitty-gritty of how checkout method is processed.

One concern would be that we're tightly coupling the SDK with Unity behavior but we could define the 'Unity' Cart interface in it's own partial class file to allow the Cart to still be used across other domains.

---

Inside the SDK, we would have our two checkout interfaces: INativeCheckout and IWebCheckout which would define StartNativeCheckout(merchantID) StartWebCheckout(url) respectfully. On cart creation, we would load up the correct implements for the platform the game is running on and forward the exposed API methods to these.

Stepping through a concrete example for web checkouts for how this might work on iOS, we would create a class named iOSWebCheckout which is responsible for the communication to that native plugin and attaches a MonoBehaviour class (maybe named iOSWebCheckoutBehaviour to the global game object to allow the native plugin side to talk back to the Unity side. When the behaviour receives information from the plugin about the state of the checkout, we would invoke the listeners attached to the Cart's UnityEvents we described above. We could do this by either passing in the Cart directly or defining another interface (ICheckoutListener) that Cart implements to forward to the UnityEvent properties.

[richardmonette]

I'm in favour of having events which the game developer can register callbacks against, seems in line with https://docs.unity3d.com/ScriptReference/UI.Button-onClick.html etc etc.

[sleroux]

Just for sake of seeing this I've updated the PR to include some of this:

https://github.com/Shopify/unity-buy-sdk/pull/114/files

[mikkoh]

I'm not sure we should use Unity Events. Here are some of the reasons why I think so.

Most events get called only happen once or twice

In this example:

cart.onCheckoutSucceded.AddListener(...);
cart.onCheckoutFailed.AddListener(...);
cart.onCheckoutCancelled.AddListener(...);

The above events will be called once maybe twice. The issue with this is that the developer will also have to handle cleaning up listeners by calling RemoveListener. If it only gets called once it's better use a callback.

Keep Unity isolated for better portability

One thing I've been attempting to strive for is for this library to be as generic as possible. What I mean by that in an ideal world this would just be the C# Buy SDK. Part of the reason to do this is that there are more and more game engines that are built on top of C#.

A good example of this is: http://www.monogame.net/showcase/

Some of our merchants are actually using this game engine. If we begin to use Unity in the core of the SDK it means that this library becomes less transportable to other environments. Currently for instance you could instantiate a ShopifyClient with a class that instatiates ILoader. This class could use MonoGame's WWW communication class. (not sure what it's actually called).

Platform familiarity isn't a necessity

When I started this library I spent some time looking at SDK's that other companies have built. I would say for eventing there is no consistent practice but using callbacks is pretty common:

https://github.com/facebook/facebook-sdk-for-unity https://developers.facebook.com/docs/unity/reference/current/FB.Init https://developers.facebook.com/docs/unity/reference/current/FB.ShareLink

https://github.com/watson-developer-cloud/unity-sdk https://github.com/watson-developer-cloud/unity-sdk#speech-to-text

[JakeCataford]

It would make sense to have a helper shim for most unity-specific things. Especially via the development of CustomPropertyDrawers for specific things. IDK if we should do this for callbacks though because it would be bad practice to have a component that delegates its error state to different components

[sleroux]

The above events will be called once maybe twice. The issue with this is that the developer will also have to handle cleaning up listeners by calling RemoveListener. If it only gets called once it's better use a callback.

Good point 👍 I can see Unity buttons using this event model since they are continuously monitoring for tap events but if we're only telling the user once per a checkout callbacks work better.

Keep Unity isolated for better portability

Moving away from UnityEvents to Delegates would make this C#-library agnostic. I think there definitely is benefit in building out Unity-specific shims as Jake mentioned but it'll be important where those files are located and how it's separated from the core SDK.

I would say for eventing there is no consistent practice but using callbacks is pretty common

True enough. Every language has its own version of a callback so I don't see this being alien.

Moving away from UnityEvents to callbacks is easy enough. I think being able to funnel anything that happens in a web or native checkout through the 3 events listed (succeeded, failed, cancelled) is the meaty part - its just a matter of how we talk to the game dev about these. I'm not sure how I feel about StartWebCheckout taking in 3 delegates as parameters though. I notice that in the Facebook API their callback returns a IShareResult parameter. I wonder if we could simply follow their model and have the API look something like:

public void CheckoutResultDelegate(ICheckoutResult result);

void StartWebCheckout(CheckoutResultDelegate callback) {

}

class CheckoutFailureResult: ICheckoutResult ...
class CheckoutSuccessResult: ICheckoutResult ...
class CheckoutCancelledResult: ICheckoutResult ...

This works in our case since all 3 of these states are mutually exclusive.

[JakeCataford]

IMO we should start with CheckoutResult and then when we need to we move to ICheckoutResult with the different implementors... also the upcasting is kinda gross in this way, i would stay its better to have base result types without a conjoining interface and have the separate delegates for each case. (more verbose, but less ambiguous from a implementation standpoint + you should be implementing all of the pathways anyways + providing different behaviour for each).

[richardmonette]

have the separate delegates for each case

[sleroux]

IMO we should start with CheckoutResult and then when we need to we move to ICheckoutResult

Actually if we pass in a 3 delegate we wouldn't even need CheckoutResult since 2 of the delegates would just be void and the failure one would have an error param:

public delegate void CheckoutDidSucceed();
public delegate void CheckoutDidFail(ShopifyError error);
public delegate void CheckoutCancelled();

void StartWebCheckout(CheckoutDidSucceed succeeded, CheckoutDidFail failed, CheckoutCancelled cancelled) {}

[sleroux]

Landed changes in https://github.com/Shopify/unity-buy-sdk/commit/22db095919ca8a7990cfd7f3bf91847c9591383c.

Thanks for all the feedback!