Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Reject acceptedPromise if converting method data fails #536

Merged
merged 7 commits into from
Jun 28, 2017
Merged

Reject acceptedPromise if converting method data fails #536

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6376b37
editorial: clarify that payment handler data is converted
marcoscaceres May 25, 2017
dae2502
Reject acceptPromise, don't rethrow
marcoscaceres May 25, 2017
18bb040
fix typos
marcoscaceres Jun 15, 2017
0a3a579
Refactor based on feedback
marcoscaceres Jun 15, 2017
927c5f0
Simplification with single identifier
Jun 21, 2017
644c14c
add in old text
marcoscaceres Jun 21, 2017
5896b12
only one identifier
marcoscaceres Jun 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 21 additions & 9 deletions index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -685,17 +685,29 @@ <h2>
<li>For each <var>paymentMethod</var> tuple in
<var>request</var>.<a>[[\serializedMethodData]]</a>:
<ol>
<li>Let <var>identifiers</var> be the first element in the <var>
paymentMethod</var> tuple.
</li>
<li>Let <var>data</var> be the second element in
<li>Let <var>identifier</var> be the first element in the
<var>paymentMethod</var> tuple.
</li>
<li>Determine which <a>payment handlers</a> support any of the
<a>payment method identifiers</a> given <var>identifiers</var>.
For each resulting payment handler, if payment method specific
capabilities supplied by the payment handler match those provided
by <var>data</var>, the payment handler matches.
<li>Let <var>data</var> be the result of <a data-cite=
"!ECMA-262-2015#sec-json.parse">JSON-parsing</a> the second
element in <var>paymentMethod</var> tuple.
</li>
<li>If required by the specification that defines the
<var>identifer</var>, then <a data-cite=
"!WEBIDL#dfn-convert-ecmascript-to-idl-value">convert</a>
<var>data</var> to an IDL value. Otherwise, <a data-cite=
"!WEBIDL#dfn-convert-ecmascript-to-idl-value">convert</a> to
<a data-cite="!WEBIDL#idl-object">object</a>.
</li>
<li>If conversion results in an error, reject
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the intent here to discard this tuple and continue (possibly logging an error in the process)?

i.e. Rejecting the promise and terminating the algorithm results in no matched payment handlers, even if there are multiple (identifer, data) tuples and only one has invalid data.

Is it possible to throw and not reject the promise so that the browser can log the error and continue the next iteration of the loop?

If so, I propose the following, or something similar:

<li>If conversion results in an error, discard
 the <var>paymentMethod</var> tuple, throw the error and process 
the next tuple.</li>

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "discard" mean? That's not a well-defined operation, on tuples or anything else...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand Adrian to be distinguishing two possible behaviors in the face of invalid data:

  • Abandon that particular payment method identifier/data pair or
  • Abandon the entire payment request

I favor the former behavior (and think AdrianHB does).

Ian

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. In that case I don't understand the suggestion. Discarding the tuple is a meaningless operation, and throwing the error is in direct contradiction to processing the next tuple (throwing an error terminates the algorithm, like it does in all programming languages).

Copy link
Collaborator

@adrianhopebailie adrianhopebailie Jun 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@domenic The algorithm is a loop through a list of tuples. I am proposing that instead of terminating the algorithm when bad data is found in one tuple, we just discard the offending tuple that contains the bad data and continue with the next.

Copy link
Collaborator

@domenic domenic Jun 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, I don't understand what "discard" means. Would your sentence mean the same thing if you said "we just continue with the next"? And what happened to "throw the error"?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@domenic please, try and understand that the majority of this WG are not experts on browser internals. We're trying our best to contribute nonetheless. Have some patience and read the whole comment.

I only made a code suggestion because the last time I tried to contribute anything @marcoscaceres refused to read it unless it had code in it.

If you re-read my original comment I asked the following question:

Is it possible to throw and not reject the promise so that the browser can log the error and continue the next iteration of the loop?
If so, I propose...

I.e. There is a parsing step happening inside a loop. Can the parsing step throw and can the logic inside the loop catch and log the error and then continue?

This is the experience I'd expect a developer to get:

  • They call the API with a list of 3 payment methods
  • They have 3 different payment handlers registered that can each handle one of these payment methods
  • One payment method has bad data
  • They are presented with only 2 payment handlers to choose to handle the payment
  • An error is logged to the browser console

I used the phrase "discard" because in the process of looping through this list of payment methods we are also establishing a list of payment handlers that match them. If a payment method has bad data then it is "discarded" from the set used for matching.

Sorry if that terminology is confusing, I hope you understand my intent now and can suggest a better text replacement?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To close the loop on this (apologies for this post merge):

We discussed this on yesterday's editor call. While there is certainly merit in ignoring the offending payment method and its related data blob, we've been moving in the direction of explicitly throwing errors for the purpose of developer clarity across this spec. It seems best to follow suit here.

We realize there are other options (e.g. console logging a warning), but as this is core to how PR works, an explicit throw was voted as the best option by the editors.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per @adrianba 's comment below, I understand the editors to have decided to take a fail fast approach to this API, therefor a payment request with bad data in one of the provided payment methods will fail entirely rather than simply logging an error.

For consistency, I recommend that the payment method identifier is also validated as part of this algorithm. It should either be a recognised short-string identifier or a valid URL form identifier (per the validity rules in the PMI spec).

Also, I wonder if this sentence in the PMI spec should be revised:
"User agents MUST NOT enforce validity or well-formedness of standardized payment method identifiers. "

<var>acceptPromise</var> with that error and terminate this
algorithm.
</li>
<li>Determine which <a>payment handlers</a> support
<var>identifier</var>. For each resulting payment handler, if
payment method specific capabilities supplied by the payment
handler match those provided by <var>data</var>, the payment
handler matches.
</li>
</ol>
</li>
Expand Down