-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Payments: Move lib/stripe to calypso-stripe package #46973
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~2113 bytes added 📈 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~1361 bytes removed 📉 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth another reviewer double checking the instances outside of the editor checkout modal, but from my point of view this change LGTM.
I've also followed all of the testing steps and seems to work fine too 😄
5f11b8e
to
040369b
Compare
Now the fetchStripeConfiguration prop will be required, allowing it to be injected from the consumer package.
This way the locale can be passed in from the consumer rather than fetching it directly from the calypso Redux store.
74f36e1
to
69bd67a
Compare
Changes proposed in this Pull Request
This moves the
lib/stripe
directory (which is used to access Stripe Elements for our credit card form fields) to a new npm package in the monorepo called@automattic/calypso-stripe
.In the process of this move, I converted the functions in that file to TypeScript (it's easiest to review that change by looking at the commits in this PR one-by-one). While doing this, I tried not to modify the function signatures at all, but a few changes were necessary, which I will list below.
StripeHookProvider
now hasfetchStripeConfiguration
as a required prop. Previously this was optional, and the default was to use the bound wpcom.js functions. However, the authentication needed for that library is entangled deeply with calypso, making it much easier to inject it from the consumer components rather than try to import it wholesale. All implementations have been updated to have this prop.StripeHookProvider
now has an optionallocale
prop. In Checkout: pass locale to Stripe (1) #43026, we added a mechanism to pass the user's locale to Stripe.js which it will use to localize any error messages it creates. However, that data comes from the calypso Redux store and is therefore tightly coupled to the calypso package. It is easier to inject the string from the consumer components rather than try to pull it directly within the new package. All implementations (other than tests, and unused components) have been updated to have this prop.This also implements the changes from #46953 and #47002
Testing instructions
Because of the move itself, as well as the modifications to
StripeHookProvider
above, this touches quite a few places in calypso. They should not be affected, but it's worth testing all of them to be sure. For the most part, if you visit these pages and the credit card form loads without any errors (either in the UI or in the console), then this should be fine./plans
, click to add a plan and you will be redirected to/checkout
)./me/purchases/add-credit-card
)./purchases/add-credit-card
)./purchases
, click on a purchase that was not made with credits, and then click "Change payment method").(This also touches old checkout for the sake of completeness, but there's no need to test that because it is unused and that code will soon be removed.)