Support a plain js config argument to payment method registration APIs #3404
Conversation
- no need for a callback dance - support the previous API: if a function is passed, call it as before
|
Size Change: +2.09 kB (0%) Total Size: 1.11 MB
ℹ️ View Unchanged
|
- add legacy fallback if passed a function - update stripe express payment method - update docs - remove unused `assertValidPaymentMethodCreator` util
haszari
changed the title
haszari
requested review from
nerrad and
senadir
and removed request for
a team
|
I'm going to do a functionality review for this soon. I'd also like us to follow up on this comment to explain that this refactor is not shipped yet and implementing it will break the integration. Also shipping it now would require people to have both latest versions at the same time, while a phase period is better. We should hold the urgency card until we actually need it (security, something blocking feature work, performance). |
|
How should the consumers of the Reading this comment made me think I'd be able to use the new argument, or the old one as fallback. The only way to do this right now would be to check The good thing is that blocks support in WCPay is still an experimental feature, so the risks of updating it to the new API without a fallback are very low. Still, is this change really necessary? The only benefit I, as some who's not really involved in the repo, see is that the registration of the PM looks a bit cleaner. |
|
@RadoslavGeorgiev I think Rua jumped the gun a bit in potentially attributing a sense of urgency to this that isn't necessary. While we will emphasize the new argument used for registration going forward, it's trivial for us to support both argument shapes for some time so WCPay can keep using the older signature until you are comfortably able to switch to the new signature (which may be what Rua meant by "as soon as practical" in his comment in the WC Pay repo). |
Totally agree, I just wanted to give WCPay a heads up. No need to make the change any time soon, both signatures will be supported for the time being. |
There was a problem hiding this comment.
Looks great Rua! I've indicated a couple minor doc fixes. Also before merging I think it'd be good to add a typedef for the payment method config option (so the passed in object shape is described by the typedef).
I didn't test this - this is a code review only.
Pre-approving - the doc fixes should be applied before merge but the typedef is up to you.
|
It seems I had a comment that got lost
You can use |
Co-authored-by: Darren Ethier <darren@roughsmootheng.in>
- use camelCase for config instance (not a constructor/type)
|
Thanks for the reviews @nerrad @senadir - great tips! This is ready to go (though feel free to review/feedback the changes) but needs testing before merge. I've tested all regular payment methods but still need a volunteer to test express payment (I can't test this).
Great idea, I've implemented this for both types, and also mentioned it in the docs. This will help us notice when we add new fields 🧑🍳. Note the type I used for the react elements is a bit fuzzy ( TIL, thanks for this. I'm now calling it if the callback is passed. It's a slightly different use case - we're deprecating an arg type, not a whole function, but it does the job. Here's the console message you get if you pass a callback:
|
|
I've got Chrome Pay working on my ephemeral test site now. I've tested the "pay now" button shows up in cart and checkout, and clicking it triggers the modal. This test confirms that express payment method registration is working correctly. Note I can't complete a successful payment with Chrome Pay - fails every time. I see this with the shortcode checkout, so I'm confident this isn't a bug with this PR :)
We can merge this when someone else has given this a quick test, ideally with a successful express payment. |
|
Tested chrome pay with incognito and it worked fine. I'm not seeing any errors related to the changes in this pr. I did reproduce what you are reporting with failed chrome pay under certain conditions for a logged in user but was also able to reproduce on trunk so it's unrelated to those changes. It's the end of my day so I'll report the issue tomorrow. |
|
Thanks @nerrad! Merging 🚢
I saw that with shortcode checkout too, so possibly unrelated to blocks. |
nerrad
added
category: extensibility
Fixes #3064
This PR updates the two APIs for registering payment methods so they are simpler. They now both receive an
optionsJS object. Previously was a callback which exposed internal details of how payment methods are handled.The new interfaces are:
registerPaymentMethod( options )registerExpressPaymentMethod( options )The existing callback API is still supported if a function is passed.
All payment methods in this code base have been updated to the new simpler API. Docs have also been updated.
This PR also includes JSDoc typedefs for the
optionsobjects. This should help us catch any payment methods passing incorrect info, and when we support new fields to options.Deprecating the old API - help!
In the issue it's suggested that we deprecate the old callback-style API. I don't know how we should do this – how do we deprecate JS APIs?
Perhaps we can add a
warn_deprecated_api()routine that logs to console, possibly with an extra conditional (e.g.SCRIPT_DEBUG?), and call this in the deprecated/legacy code path.Update: using
@wordpress/deprecated.API docs
I've manually updated the markdown doc for now. Long term it might be a good idea to start documenting our JS APIs using code docs, for example using JSDoc or some other tech. Then we can use this to automatically generate consistently formatted API docs, and also keep track of changes over different releases.
@nerrad - keen for your thoughts on this. If there's interest I'm happy to take a look in the next cooldown.
I haven't looked to see what other projects are doing (e.g. Gutenberg, WooCommerce, wc-admin, Jetpack), maybe there's a convention we can adopt.
Update: using a JSDoc typedef now.
How to test the changes in this Pull Request:
Note I haven't tested the following payment methods myself, would appreciate extra testing of the following. Please comment in your review which payment methods you tested – thanks!
Bonus points:
Changelog
Developer Note
Payment method registration APIs receive JS options arg (was callback)
In WooCommerce Blocks
{version}the APIs for registering a payment method have changed.The affected JavaScript functions are:
registerPaymentMethod( options )registerExpressPaymentMethod( options )Previously these APIs received a callback function, for example:
Now you can simply pass the options object directly:
All payment methods (gateway extensions) should update to the new API as soon as possible. The previous callback-based API is supported as a fallback, and will be deprecated in a future release.