- From: Domenic Denicola <notifications@github.com>
- Date: Fri, 25 Aug 2017 20:59:42 +0000 (UTC)
- To: w3c/payment-request <payment-request@noreply.github.com>
- Cc: Subscribed <subscribed@noreply.github.com>
- Message-ID: <w3c/payment-request/pull/591/review/58759335@github.com>
domenic commented on this pull request.
> @@ -2400,13 +2405,59 @@
<h2>
<dfn data-lt="updateWith(detailsPromise)">updateWith()</dfn> method
</h2>
- <p class="note">
- If a developer wants to update the payment request, then they need
- to call <a>updateWith()</a> and provide a
- <a>PaymentDetailsUpdate</a> dictionary, or a promise for one,
- containing changed values that the <a>user agent</a> presents to
- the user.
- </p>
+ <aside class="note">
+ <p>
+ If a developer wants to update the payment request, then they
I'm just noticing that this repeats text from slightly above. Let's delete the above text, as it feels pretty out of place anyway.
> + changes made by the end-user through the UI), developers need to
+ immediately call <a>updateWith()</a>. If they don't,
+ <a>[[\didUpdate]]</a> becomes true and <a>updateWith()</a>
+ will throw.
+ </p>
+ <pre class="example" title="how to use `updateWith()` correctly.">
+ // ❌ Bad - this won't work!
+ request.onshippingaddresschange = async ev => {
+ // `await` goes to next tick, so [[didUpdate]] is now true.
+ const details = await Promise.resolve({ ...details, ...updatedProps });
+ // 💥 Too late, throws "InvalidStateError".
+ ev.updateWith(details);
+ };
+
+ // ✅ Good - UI will wait.
+ request.onshippingaddresschange = async ev => {
No `async` here
> + immediately call <a>updateWith()</a>. If they don't,
+ <a>[[\didUpdate]]</a> becomes true and <a>updateWith()</a>
+ will throw.
+ </p>
+ <pre class="example" title="how to use `updateWith()` correctly.">
+ // ❌ Bad - this won't work!
+ request.onshippingaddresschange = async ev => {
+ // `await` goes to next tick, so [[didUpdate]] is now true.
+ const details = await Promise.resolve({ ...details, ...updatedProps });
+ // 💥 Too late, throws "InvalidStateError".
+ ev.updateWith(details);
+ };
+
+ // ✅ Good - UI will wait.
+ request.onshippingaddresschange = async ev => {
+ // This is fine, of course.
"of course" is a bit aggressive and implies the reader understands what's going on. You shouldn't really say "of course" while teaching someone something like this, IMO.
> + <a>PaymentDetailsUpdate</a> dictionary, or a promise for one,
+ containing changed values that the <a>user agent</a> presents to
+ the user.
+ </p>
+ <p>
+ To prevent the user interface from blocking (and to reflect
+ changes made by the end-user through the UI), developers need to
+ immediately call <a>updateWith()</a>. If they don't,
+ <a>[[\didUpdate]]</a> becomes true and <a>updateWith()</a>
+ will throw.
+ </p>
+ <pre class="example" title="how to use `updateWith()` correctly.">
+ // ❌ Bad - this won't work!
+ request.onshippingaddresschange = async ev => {
+ // `await` goes to next tick, so [[didUpdate]] is now true.
+ const details = await Promise.resolve({ ...details, ...updatedProps });
Let's do `await getNewDetails(oldDetails)` to mirror the below.
> + <aside class="note">
+ <p>
+ If a developer wants to update the payment request, then they
+ need to call <a>updateWith()</a> and provide a
+ <a>PaymentDetailsUpdate</a> dictionary, or a promise for one,
+ containing changed values that the <a>user agent</a> presents to
+ the user.
+ </p>
+ <p>
+ To prevent the user interface from blocking (and to reflect
+ changes made by the end-user through the UI), developers need to
+ immediately call <a>updateWith()</a>. If they don't,
+ <a>[[\didUpdate]]</a> becomes true and <a>updateWith()</a>
+ will throw.
+ </p>
+ <pre class="example" title="how to use `updateWith()` correctly.">
Backticks don't work in example titles. Respec bug? Or just remove it for now?
Same below.
> + // 💥 Too late, throws "InvalidStateError".
+ ev.updateWith(details);
+ };
+
+ // ✅ Good - UI will wait.
+ request.onshippingaddresschange = async ev => {
+ // This is fine, of course.
+ const promiseForNewDetails = getNewDetails(oldDetails);
+ ev.updateWith(promiseForNewDetails);
+ };
+ </pre>
+ <p>
+ Additionally, <a>[[\didUpdate]]</a> prevents reuse of
+ <a>PaymentRequestUpdateEvent</a>.
+ </p>
+ <pre class="example" title="can only call `upadteWith()` once">
In addition to the backticks, updateWith() is misspelled
> + request.onshippingaddresschange = async ev => {
+ // `await` goes to next tick, so [[didUpdate]] is now true.
+ const details = await Promise.resolve({ ...details, ...updatedProps });
+ // 💥 Too late, throws "InvalidStateError".
+ ev.updateWith(details);
+ };
+
+ // ✅ Good - UI will wait.
+ request.onshippingaddresschange = async ev => {
+ // This is fine, of course.
+ const promiseForNewDetails = getNewDetails(oldDetails);
+ ev.updateWith(promiseForNewDetails);
+ };
+ </pre>
+ <p>
+ Additionally, <a>[[\didUpdate]]</a> prevents reuse of
[[didUpdate]] and [[waitForUpdate]] together prevent reuse
> + };
+
+ // ✅ Good - UI will wait.
+ request.onshippingaddresschange = async ev => {
+ // This is fine, of course.
+ const promiseForNewDetails = getNewDetails(oldDetails);
+ ev.updateWith(promiseForNewDetails);
+ };
+ </pre>
+ <p>
+ Additionally, <a>[[\didUpdate]]</a> prevents reuse of
+ <a>PaymentRequestUpdateEvent</a>.
+ </p>
+ <pre class="example" title="can only call `upadteWith()` once">
+ // ❌ Bad - calling updateWith() doesn't work!
+ request.addEventListener("shippingaddresschange", async ev => {
no `async`
> + need to call <a>updateWith()</a> and provide a
+ <a>PaymentDetailsUpdate</a> dictionary, or a promise for one,
+ containing changed values that the <a>user agent</a> presents to
+ the user.
+ </p>
+ <p>
+ To prevent the user interface from blocking (and to reflect
+ changes made by the end-user through the UI), developers need to
+ immediately call <a>updateWith()</a>. If they don't,
+ <a>[[\didUpdate]]</a> becomes true and <a>updateWith()</a>
+ will throw.
+ </p>
+ <pre class="example" title="how to use `updateWith()` correctly.">
+ // ❌ Bad - this won't work!
+ request.onshippingaddresschange = async ev => {
+ // `await` goes to next tick, so [[didUpdate]] is now true.
To distinguish this from the last example, we should say something like "await goes to next tick, and updateWith() was not called before that, so [[didUpdate]] gets set to true".
Although that calls into question whether didUpdate is the right name, since nothing actually updated.
> + <a>PaymentRequestUpdateEvent</a>.
+ </p>
+ <pre class="example" title="can only call `upadteWith()` once">
+ // ❌ Bad - calling updateWith() doesn't work!
+ request.addEventListener("shippingaddresschange", async ev => {
+ ev.updateWith(details); // this is ok.
+ // 💥 [[waitForUpdate]] is true, throws an "InvalidStateError".
+ ev.updateWith(otherDetails);
+ });
+
+ // ❌ Bad - this won't work either!
+ request.addEventListener("shippingaddresschange", async ev => {
+ const p = Promise.resolve({ ...details });
+ ev.updateWith(p);
+ await p;
+ // 💥 [[didUpdate]] is now true, throws "InvalidStateError".
To distinguish this from the first example, we can say "the update has now completed, so [[didUpdate]] gets set to true; this throws "InvalidStateError".
> @@ -2880,9 +2939,24 @@
further action. The <a>user agent</a> user interface should ensure
that this never occurs.
</li>
+ <li>Let <var>event</var> be the result of <a data-cite=
+ "!DOM#concept-event-create">creating an event</a> using
+ <a>PaymentRequestUpdateEvent</a>.
+ </li>
+ <li>Initialize <var>event</var>'s type attribute to <var>name</var>.
Ideally link type, or at least `<code>`ify it.
> <li>
- <a>Fire an event</a> named <var>name</var> at <var>request</var>
- using <a>PaymentRequestUpdateEvent</a>.
+ <a data-cite="!DOM#concept-event-dispatch">Dispatch</a>
+ <var>event</var> at <var>request</var>.
+ </li>
+ <li data-link-for="PaymentRequestUpdateEvent">If
+ <var>event</var>.<a>[[\waitForUpdate]]</a> is true, disable any part
+ of the user interface that could cause another update event to be
+ fired. Wait for <var>event</var>.<a>updateWith()</a> method's
+ <var>detailsPromise</var> to settle.
event.updateWith()'s detailsPromise is pretty terrible. Can we store it in an internal slot?
Actually, what is the point of this "wait" step? Nothing happens after it. I think it should just be removed.
--
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/591#pullrequestreview-58759335
Received on Friday, 25 August 2017 21:00:10 UTC