Re: [w3c/payment-request] WIP - very rough sketch of requestShippingAddress() method (#873)

danyao commented on this pull request.



> +            follows:
+          </p>
+          <ol class="algorithm">
+            <li>Let |event:PaymentMethodChangeEvent| be this
+            {{PaymentMethodChangeEvent}} instance.
+            </li>
+            <li>If |event|'s {{ Event.isTrusted }} attribute is false, then
+            return <a>a promise rejected with</a> an {{"InvalidStateError"}}
+            {{DOMException}}.
+            </li>
+            <li>Let |request:PaymentRequest| be the value of |event|'s
+            [=Event/target=].
+            </li>
+            <li>Assert: |request| is an instance of {{PaymentRequest}}.
+            </li>
+            <li>If |request|.<a>[[\state]]</a> is not "<a>interactive</a>",

> Should calling `ev.requestBillingAddress()` more than once inside `ev.updateWith()` reject? Or should the browser UI just deal with all those?

Multiple calls that are chained as a thenable should be allowed, but parallel calls should not. I introduced a new Internal Slot for both event types to enforce this.

As after discussing @rsolomakhin, I don't think there's actually a way to check that `requestBillingAddress()` is only called "inside" `updateWith()`, because by "inside", we actually mean that `requestBillingAddress()` be called inside the Promise that is passed to `updateWith()`. From an expression evaluation order perspective, these two are identical:

```
// requestBillingAddress() called "inside" updateWith():
ev.updateWith(async () => {
  const methodDetails = await ev.requestBillingAddress(["country"]);
  const total = await getNewTotal(methodDetails);
  return total;
});

// which is identical to:
let p = ev.requestBillingAddress(["country"])
  .then(methodDetails => {
    return getNewTotal(methodDetails);
  });
ev.updateWith(p);
```

The best solution I can come up with is to require `requestBillingAddress()` be called *before* `updateWith()` (by checking `ev.[[waitForUpdate]]` is not true in `requestBillingAddress()` algorithm). The only case this doesn't cover is if `ev.requestBillingAddress(parts)` is called without calling `ev.updateWith()`. In this case, the payment handler should update its internal states and return a billing address that includes `parts` in the final response.

UI is a bit tricky. I think at a minimum `requestBillingAddress()` should only be callable when the payment handler UI is showing so that the merchant is only able to request address as part of an active transaction with a payment handler. The current algorithm enforces this by checking `request.[[state]]` is "interactive". I'm not sure if it makes sense to also spec a user prompt that either the browser of the payment handler needs to show for every `requestBillingAddress()` call. It seems to expose too much implementation details to the user.

> Right, yes, calling ev.requestBillingAddress() per `.updateWith()`, with multiple `.updateWith()` calls makes sense.. right now it's not clear what one would do in the UI with:
> 
> ```js
> ev.updateWith((async () => {
>    const p1 = ev.requestBillingAddress(["country"]);
>    await new Promise(r => setTimeout(r, 2000));
>    const p2 = ev.requestBillingAddress(["postcode"]);
>    await new Promise(r => setTimeout(r, 2000));
>    const p3 = ev.requestBillingAddress(["city", "state"]);
>    await Promise.all([p1, p2, p3]);
>    return someNewTotal();
> }));
> ```

Yeah I think this case should not be allowed because the three calls are "in parallel". I introduced a new internal slot on both event types to check this and also expanded the code examples to cover all known good and bad patterns.




-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/w3c/payment-request/pull/873#discussion_r345563739