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.

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

Add isSecurePaymentConfirmationAvailable API to spec #233

Merged
merged 7 commits into from
Jun 20, 2023

Conversation

nickburris
Copy link
Member

@nickburris nickburris commented Mar 9, 2023

This adds spec text for an API for developers to more easily check whether SPC is available. Fixes #81. One question is whether the spec should further clarify what it means for SPC to be available, but I'm not sure how we can say that without being too implementation specific.


Preview | Diff

@stephenmcgruer stephenmcgruer marked this pull request as ready for review March 10, 2023 16:53
Copy link
Collaborator

@stephenmcgruer stephenmcgruer left a comment

Choose a reason for hiding this comment

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

I think this is roughly fine as-is, but cc @ianbjacobs who may have an opinion.

Can you update the example in section 1.2.2 to: (1) use your const spcAvailable = ..., and to (2) remove or update the comment about canMakePayment ?

Actually, what this means for canMakePayment is a good question. Payment Request is notoriously vague about that method - the algorithm just says:

If the user agent has a [payment handler](https://w3c.github.io/payment-request/#dfn-payment-handler) that supports handling payment requests for identifier, resolve hasHandlerPromise with true and terminate this algorithm.

(Note, as always, that the Steps to check if a payment can be made concept is unrelated to canMakePayment and is actually part of show() -_-.)

So I think overall we should just remove the canMakePayment usage in example 1.2.2, and discourage use of canMakePayment for SPC, in preference for this static method?

@ianbjacobs
Copy link
Collaborator

Thanks @nickburris and @stephenmcgruer,

I don't have a strong view; let's try to get input from users of the API.

Having said that, I see some small advantages to isSecurePaymentConfirmationAvailable:

  1. isSecurePaymentConfirmationAvailable appears to be more straightforward in meaning than canMakePayment.
  2. This new approach might ease a transition away from SPC-via-Payment-Request if we choose that path.

@stephenmcgruer
Copy link
Collaborator

I don't have a strong view; let's try to get input from users of the API.

How/when do you want to gather this input?

Having said that, I see some small advantages to isSecurePaymentConfirmationAvailable: [...]

Yes, I think we definitely want to make isSecurePaymentConfirmationAvailable the primary way to do availability-checking for the API. The question is more "what about canMakePayment" in such a world - should the SPC spec try to address it (either by defining it to do 'something' useful, TBD what, or by explicitly saying not to use it for SPC)?

@ianbjacobs
Copy link
Collaborator

I don't have a strong view; let's try to get input from users of the API.

How/when do you want to gather this input?

I have reached out to Stripe and Adyen colleagues for input. I'll also add this to our next meeting agenda.

Having said that, I see some small advantages to isSecurePaymentConfirmationAvailable: [...]

Yes, I think we definitely want to make isSecurePaymentConfirmationAvailable the primary way to do availability-checking for the API. The question is more "what about canMakePayment" in such a world - should the SPC spec try to address it (either by defining it to do 'something' useful, TBD what, or by explicitly saying not to use it for SPC)?

While SPC is grounded in PR API I don't see a reason to try to get rid of canMakePayment. So would this make sense language for users of the API:

  • Use isSecurePaymentConfirmationAvailable
  • canMakePayment is the old way of doing this; this approach is deprecated.
  • Implementations will continue to support both (until the mythical day when SPC is no longer grounded in PR API, at which point canMakePayment could naturally fall away).

@stephenmcgruer
Copy link
Collaborator

I have reached out to Stripe and Adyen colleagues for input. I'll also add this to our next meeting agenda.

Ack. Note that we would like to ship this in M113 (branches March 23rd, for stable release in early May), so we are likely to pursue that anyway as we don't foresee any objections from the WG. If for some reason there is significant push-back at the remote meeting (week of March 27th), we should be fine to halt the brakes that close post-branch.

While SPC is grounded in PR API I don't see a reason to try to get rid of canMakePayment. So would this make sense language for users of the API:

Yep, sgtm. Practically, what I believe this amounts to is:

  1. Spec isSecurePaymentConfirmationAvailable
  2. Add a spec note saying "canMakePayment also exists, but we recommend using isSecurePaymentConfirmationAvailable instead"

Does that SGTY @ianbjacobs ?

@omertoast
Copy link

  1. Spec isSecurePaymentConfirmationAvailable
  2. Add a spec note saying "canMakePayment also exists, but we recommend using isSecurePaymentConfirmationAvailable instead"

SGTM. As someone who didn't know this issue exist, it seems straightforward and simple as it gets.

@adrianhopebailie
Copy link
Collaborator

until the mythical day when SPC is no longer grounded in PR API

< sigh />

@ianbjacobs
Copy link
Collaborator

until the mythical day when SPC is no longer grounded in PR API

< sigh />

It's an open issue; sorry I was a bit blithe in my comment.

@nickburris
Copy link
Member Author

Can you update the example in section 1.2.2 to: (1) use your const spcAvailable = ..., and to (2) remove or update the comment about canMakePayment ?

Done.

Copy link
Collaborator

@stephenmcgruer stephenmcgruer left a comment

Choose a reason for hiding this comment

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

LGTM with one comment and one optional comment. Thanks Nick!

spec.bs Outdated Show resolved Hide resolved
spec.bs Show resolved Hide resolved
@stephenmcgruer
Copy link
Collaborator

@ianbjacobs - PTAL as well. Let us know also if you want this to wait for a WPWG meeting to let anyone chime in, or if you're happy to see it merged after your review.

spec.bs Outdated Show resolved Hide resolved
@ianbjacobs
Copy link
Collaborator

Hi @nickburris,

I've approved the request, but this should not be part of the document that we advance to Candidate Recommendation. I do expect us to fold in other pull requests that will be part of the CR (namely, #230, #234, #236, and #238). Should we block merging this for now?

@stephenmcgruer
Copy link
Collaborator

Should we block merging this for now?

Yep; let's just leave this PR sitting until the first version of SPC gets to CR (assuming no significant bumps along the way). On the Chrome side, an open approved PR is sufficient for our launch processes :)

@ianbjacobs ianbjacobs added the Blocked Don't merge until a dependency has been resolved label Apr 11, 2023
@RByers
Copy link

RByers commented Apr 25, 2023

[Chromium API owner hat on] Do y'all have an idea of how long you expect SPC to take to get to CR? While we can ship based on a PR if necessary, it's not ideal as it can contribute to confusion about what's ready to be implemented in other engines. If you expect it to take more than a month or so, could you consider creating a separate 'v2' branch or something which we can point to as the landed spec for this API?

@ianbjacobs
Copy link
Collaborator

Hi @RByers,
We are poised to request to advance to CR. I am hopeful for a mid-May publication.

@RByers
Copy link

RByers commented Apr 25, 2023

Thanks @ianbjacobs. Leaving this as an approved but unlanded PR for a few weeks seems like a reasonable pragmatic tradeoff to me. If it ends up taking more than a month or two, would you be open to the future/v2 branch idea?

@ianbjacobs
Copy link
Collaborator

@RByers, yes, very open to a versioning / branch discussion.

@stephenmcgruer
Copy link
Collaborator

@ianbjacobs - now that we have a CR version branched (though not approved), what do you think of landing this PR? I don't expect us to have many more changes to bring into the CR branch, and if we do we can cherry-pick them individually from main as needed.

@ianbjacobs
Copy link
Collaborator

@stephenmcgruer,
I'm ok to land the pull request, but would prefer to do so after the publication of the CR (ideally, therefore, in a few days). Would it be ok to wait for that? If you want to delegate landing this to me (for just after the publication), I'm happy to merge it.

@stephenmcgruer
Copy link
Collaborator

Sure, sounds good to me - if you can take the task to land this once the CR is published, SGTM. Thanks! :)

@ianbjacobs ianbjacobs merged commit 200fb61 into w3c:main Jun 20, 2023
@ianbjacobs ianbjacobs removed the Blocked Don't merge until a dependency has been resolved label Jun 20, 2023
github-actions bot added a commit that referenced this pull request Jun 20, 2023
SHA: 200fb61
Reason: push, by ianbjacobs

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Jun 20, 2023
SHA: 200fb61
Reason: push, by ianbjacobs

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Jun 21, 2023
SHA: 200fb61
Reason: push, by ianbjacobs

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make it easy to determine whether a user agent supports SPC
6 participants