Use navigator.payments singleton, factory method, or PaymentRequest constructor

[adrianba]

This issue comes from WICG/paymentrequest#42.

From the spec:

There is an open issue about how a payment request is initiated. The options proposed include using a singleton at navigator.payments, using a factory method to create an instance, or using a constructor as currently described in the specification.

[adrianhopebailie]

In TAG review @triblondon said:

I prefer a constructor. I'd definitely say no to a singleton unless it was a solid certainty that we'll never want a UA to be processing multiple payments at a time, since the instance provides a means of storing state which seems like it would become more useful as the API is extended in future.

[adrianhopebailie]

@msporny said:

To clarify, the singleton would instantiate a Promise and return it. You'd be able to run multiple payment requests in a page. I'm not strongly arguing for/against - just clarifying.

[adrianhopebailie]

Part of the problem with using the constructor is that we have no namespaced place to hang other API methods that we are still working on for things like registration, getting the PaymentRequest and submitting the PaymentResponse as a web based payment app.

A factory pattern might be a good compromise?

Proposal

Put the PaymentRequest constructor behind a factory method.

Sample javascript:

var payment = navigator.payments.newPaymentRequest(supportedMethods, details, options, data);
payment.addEventListener("shippingaddresschange", function (changeEvent) {
    // Process shipping address change
});

payment.show().then(function(paymentResponse) {
  // Process paymentResponse
  // paymentResponse.methodName contains the selected payment method
  // paymentResponse.details contains a payment method specific response
  paymentResponse.complete(true);
}).catch(function(err) {
  console.error("Uh oh, something bad happened", err.message);
});

[triblondon]

Couldn't you use static methods of the PaymentRequest constructor if you wanted to create new non-instance methods in future? But regardless the factory seems fine. The web platform has pretty much no consensus or consistency in this regard so there isn't a strong convention, imho.

[sideshowbarker]

The web platform has pretty much no consensus or consistency in this regard so there isn't a strong convention, imho

Finding out what the recommended best practices are for details like this is a big part of the motivation of asking for close review from the TAG. But if the answer really is that there really is no consensus or consistency in this particular case, then that actually is also useful to know.

[triblondon]

It would be worth raising this as a specific question on the TAG mailing list to get a broader opinion from the whole group.

[msporny]

Put the PaymentRequest constructor behind a factory method.

This (interface singleton w/ factory methods) is exactly what the Web Payments Community Group spec proposal does, partly for the reasons outlined by @adrianhopebailie:

http://wicg.github.io/web-payments-browser-api/#navigatorpayment

and

http://wicg.github.io/web-payments-browser-api/#processing-a-payment-request

So, +1 for this design direction.

If folks want to -1 this, then I'd be interested in hearing where we're going to put methods to do registration, receipt storage (potential future feature), pickup of pending payee-initiated payment requests on the payment app side, etc.

[adrianhopebailie]

Couldn't you use static methods of the PaymentRequest constructor if you wanted to create new non-instance methods in future?

Are you suggesting a style like this:

var thing = PaymentRequest.someMethod();

My only reservation is that someMethod may be more generically related to payments as opposed to payment requests (eg: registration of a payment app) in which case this doesn't make sense.

I'm certainly more familiar with (and prefer) the pattern:

var thing = navigator.payments.someMethod();
var paymentRequest = navigator.payments.createRequest();

I can also imagine us having API endpoints like the following that are used by web-based payment apps:

//After the payment app loads ask the browser for the current request that it must process
var paymentRequest = navigator.payments.getRequest();
//Submit back the response
paymentRequest.submitResponse(paymentResponse);

Or, if the request is passed to the app as an HTTP request then:

//Submit back the response
navigator.payments.submitResponse(paymentResponse);

[adrianba]

Here are my thoughts on this issue.

I interpret singleton in this context to mean that we use the same object for each payment request. This would mean that, if navigator.payments is the singleton object, then you could write navigator.payments.addEventListener(…) and this would fire for all payment requests.

The problem with this pattern is that you have to understand how to reset a payment request. Currently the PaymentRequest object is a one-time deal. The state transitions show that you end up in closed and there are no state transitions leading back to earlier states. Reusing objects for new requests has been a source of interop problems in the past (e.g., with XHR) and we should keep this simple and avoid object reuse. This means we should not have a singleton for a payment request.

I can live with a factory method if that is really what people want. This could be navigator.createPaymentRequest() or PaymentRequest.create(). In my mind, factory methods are useful in two key scenarios. 1) When creating an instance of the object binds that object to some internal state that isn't exposed to calling code. The factory method implicitly retrieves that state and binds it to the instance; or 2) when the creation of the object causes some significant processing to occur. The factory method makes it clear in that case that this isn't simple object creation.

For our API, the constructor simply validates its arguments. It creates an instance with the validated arguments. It seems fine to use a constructor in this way. There are plenty of types in the browser type system that use constructors today and we've been moving away from unnecessary factory patterns in recent times (e.g. Events). This was the reason we moved to a constructor in the original proposal.

I would object to using a singleton but otherwise we should just decide quickly and make this issue go away. I don't see a need to change but if there is a quick consensus to do something else then we should do that now. Otherwise this is just bikeshedding and adding uncertainty to implementation.

[dlongley]

@adrianba,

In my mind, factory methods are useful in two key scenarios. 1) When creating an instance of the object binds that object to some internal state that isn't exposed to calling code. The factory method implicitly retrieves that state and binds it to the instance; or 2) when the creation of the object causes some significant processing to occur. The factory method makes it clear in that case that this isn't simple object creation.

3. When you want to override the factory method to return back a subclass of the original class that extends the original behavior in some way (or a different implementation for a common interface). In this case, no code that uses the factory needs to change, only the factory method gets updated.

I also like the idea of keeping new classes out of the global namespace (but this is not specific to factory methods).

[adrianba]

I also like the idea of keeping new classes out of the global namespace (but this is not specific to factory methods).

How do you keep interfaces out of the global namespace?

[dlongley]

https://dev.w3.org/geo/api/spec-source.html#geolocation_interface

typeof Geolocation === 'undefined'

[adrianba]

@dlongley That's a singleton.

[adrianba]

I think we're going to need to use the interface object for feature detection so we need to include it in the type system.

[dlongley]

We can make payments a singleton that hangs off navigator like this new API does:

https://w3c.github.io/webappsec-credential-management/#interfaces-credential-manager

And then add a createPaymentRequest factory function to it that returns a PaymentRequest. An instance of PaymentRequest would not be a singleton. To check for feature detection you look for payments on navigator -- and if there are other features that we add in the future, you check for them off of payments.

[adrianba]

If you're adding createPaymentRequest then you don't need payments. Binary existence checking won't be sufficiently granular for feature detection as we evolve the API. This is why we need an interface to statically interrogate.

[adrianhopebailie]

While more of s style argument I do think there is value in navigator.payments as a way of namespacing the payments functions.

Assuming we may have others like registerPaymentApp() in future it seems sensible to do this.

It's a personal preference and I'm open to being persuaded this is old-fashioned

[msporny]

This means we should not have a singleton for a payment request.

Upon re-reading all the threads, I don't think anyone proposed a singleton object with shared internal state as a solution? So, I see no support for that, which is good, because I think we're all in agreement on that point.

I think what's being proposed is this general shape:

navigator.payment Proposal

partial interface Navigator {
  readonly attribute PaymentsInterface payment;
};

interface PaymentsInterface {
  Promise<PaymentResponse> request(..., options); // this is used by a merchant
  Promise<PaymentRequest> getPendingRequest(); // this is used by a payment app
};

Example usage

To create a payment request, you do this:

var pr = navigator.payment.request(...);

instead of this:

var pr = new PaymentRequest(...);

Not much difference here, other than how much one has to type. So, let's keep looking for some other usages of the API, like getting pending requests (something a 3rd party Payment App would have to do):

// the payment app will need to make this call when it is invoked
var pr = navigator.payment.getPendingRequest();

It's true that we could do it like this:

// the payment app will need to make this call when it is invoked
var pr = PaymentRequest.getPendingRequests();

Again, we could do it either way, no strong argument for one over the other yet.

Now let's add a feature that doesn't really have to do with Payment Requests but does have to do with the payments ecosystem:

var reg = navigator.payment.registerApp(...);

and this is how this might be handled with the current spec approach:

var reg = PaymentRequest.registerApp(...);

Now it starts getting a bit strange. Why are we calling the app registration method on PaymentRequest? What does registration have to do with the PaymentRequest interface? This same argument will come up if/when we add any functionality that has a tenuous connection to PaymentRequest. For example, how do we initiate the storage of digital receipts?

var save = PaymentRequest.storeReceipt(...);

At this point, PaymentRequest stops being just about payment requests and is instead starting to be used as "the Payment Request API static object". To put it another way, we have two things that we're talking about here:

1. The functionality that is available via the Web Payments ecosystem.
2. The functionality that is available via a Payment Request object.

The current specification attempts to shove both items above into PaymentRequest and that seems like a bad thing to do from a design perspective because it turns something that's meant to be a fairly clean and simple interface (PaymentRequest) into a grab bag of functionality (everything that you can do in the Web Payments ecosystem).

So, by using the proposal above, it will help organize methods mostly unrelated to PaymentRequests under an easily understandable namespace 'navigator.payment.'

[dlongley]

@adrianba,

Binary existence checking won't be sufficiently granular for feature detection as we evolve the API.

As we evolve the API in what way? Can you give some examples of what you'd like to query for (including non-binary checks)?

I'm also not convinced that it would be a good thing to have to interrogate an interface for things like 'requestPayerEmail'. In the future would I be checking for requestPayerFoo, requestPayerBar, request_1, ... request_N? If that's a possibility, that is, in my view, evidence that we're doing something wrong. I think, in those situations, it would be better to be able to ask the API customerInfo.supports('arbitrary-identifier') and do something more generic and extensible.

What other examples of feature detection are you referring to? It would be easier to respond if we can ground this in some concrete examples.

[rsolomakhin]

+1 to @adrianhopebailie on namespacing under navigator.payments. +1 and @adrianba to avoid factories. I would prefer something like this:

var request = navigator.payments.request(supportedMethods, paymentDetails);
request.addEventListener("eventname", e => {/*....*/));
request.show().then(instrumentResponse => {/*....*/}).catch(error => {/*....*/});

Then we can place new methods (e.g., registration) under the navigator.payments method as well.

navigator.payments.register(appData);

[rsolomakhin]

+1 to avoid singletons as well.

I realize that navigator.payments.request() in my previous example is really a factory because of absence of new. I am OK with new navigator.payments.request() syntax as well.

[adrianba]

I've spent some time trying to change the proposals here into a pull request that I can live with but after looking at a couple of options I'm not currently a supporter of this change.

One goal of the current design is that using the constructor signifies lightweight object creation with no processing other than synchronous validation of the arguments. This means you can construct a PaymentRequest whenever you want and it's just an object in memory. If you choose then you can attach event handlers either using EventTarget.addEventListener() or using the function attributes on the object. Nothing really happens until you call show() and at this point you get a promise and asynchronous processing happens on the arguments passed in.

Calling navigator.payments.request suggests that you are actually making the request. In fact, one of the proposals above does return the promise that show() gives you so it appears to be combining the constructor and show() methods. Since there is no way to synchronously get the PaymentRequest object, you can't add event listeners without there being a risk of missing events.

I toyed with the idea of using navigator.payments.createRequest but even this name could easily be confused with make request and doesn't signify the only the construction of an in memory object.

We have lots of constructors in our type system today. Fairly recently, Events moved away from using the initEvent() method to using a constructor. Modern specs like WebRTC, Web Notifications, and Fetch use constructors so it's not necessarily considered a pattern to avoid. This is an approach that developers are used to and generally signifies creating an object with no processing, which is what we want.

With this in mind I propose that we keep the current API, at least for now until we learn more.

[adrianhopebailie]

@adrianba - This makes sense to me, I am happy to leave this as is.