Skip to content

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

Open
adrianba opened this Issue Mar 3, 2016 · 19 comments
@adrianba
adrianba commented Mar 3, 2016

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.

@mattsaxon mattsaxon added the Technical label Mar 21, 2016
@mattsaxon mattsaxon added this to the Priority: Medium milestone Mar 21, 2016
@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
World Wide Web Consortium member

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
@msporny
msporny commented Apr 8, 2016

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
adrianba commented May 5, 2016

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
dlongley commented May 5, 2016 edited

@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
adrianba commented May 5, 2016

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?

@adrianba
adrianba commented May 5, 2016

@dlongley That's a singleton.

@adrianba
adrianba commented May 5, 2016

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
dlongley commented May 5, 2016 edited

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
adrianba commented May 5, 2016

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
msporny commented May 5, 2016 edited

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
dlongley commented May 5, 2016

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.