-
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
Checkout: Remove CartStore from checkout #50681
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~58 bytes removed 📉 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~468 bytes removed 📉 [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 (~264 bytes added 📈 [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. |
@Automattic/martech-decepticons and @southp I could really use your assistance with this PR. I don't know enough about how siteless and registrationless checkout works to be able to test or debug these changes. |
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.
Code-wise, this looks great.
I tested the /no-user/
flow using the instructions provided, purchasing both a domain and plan, and then repeated purchasing a plan and a post-Checkout concierge session. Everything worked as expected.
I then tested the /no-site/
variant by visiting /start/domain
and selecting "just a domain" in the second step. Everything worked as expected.
If there are no objections from @niranjan-uma-shankar, I'd say that we're good to merge this.
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/5577880 Hi @sirbrillig, could you please edit the description of this PR and add a screenshot for our translators? Ideally it'd include this string: Thank you in advance! |
The testing instructions worked. Here are a few more scenarios I tried: ✅ Scenario 1 - Log in to an account with no site
❌ Scenario 2 - Log in to an account with a single site
❌ Scenario 3 - Reload on the logged out checkout page
|
Thank you! I followed the testing instructions for both of the failing cases above with a commit before this was merged ( Alternatively, if you could describe how the underlying functionality worked, I can try to debug it myself, but there's a lot I don't understand right now. |
Ah okay, if that's the case, then we can ignore it.
Appreciate your offer to debug it, but I agree with your first statement that it should be a task for MarTech, so please don't bother 😄 . There's a lot that has changed in the checkout code and other places since the time registrationless checkout was first implemented, so it's not surprising that it breaks for some cases. I'll debug it as part of our upcoming efforts to re-launch registrationless checkout(discussed in pau2Xa-2JW-p2). |
Translation for this Pull Request has now been finished. |
Changes proposed in this Pull Request
As a final piece to remove the deprecated CartStore (see #24019) we need to remove the
CartData
wrapper aroundCheckoutSystemDecider
. That wrapper was added to guard against race conditions where other parts of calypso might be using the CartStore to modify the cart and the new checkout (specifically theCalypsoShoppingCartProvider
) needed to wait until those transactions completed before initializing. Since no other parts of calypso still use CartStore to modify the cart, this race condition guard can be removed.However, after the guard was put in place, it was also used in #44206 as a mechanism to inject cart items from localStorage as the initial cart. This was done so that siteless and registrationless signup could prepare items to be added to the cart before the shopping-cart endpoint would allow adding those items. The way this works (if I understand it correctly) is that signup writes the temporary items to localStorage and those items are then read by the CartStore into its internal cache which it then posts to the shopping-cart endpoint (every 20 seconds, endlessly, I believe).
CheckoutSystemDecider
then replaces theCalypsoShoppingCartProvider
fetcher with a hard-coded Promise that resolves to the updated card from the CartStore, so that when checkout goes to pull in the initial copy of the cart, it receives the products written by signup instead.This strategy has a problem as we try to migrate it away from using CartStore. The cart items written to localStorage (and indeed the rest of the cart) are missing many required properties set by the shopping-cart endpoint (they do not match the
ResponseCartProduct
type), and therefore cannot be safely used in checkout without a round-trip to the endpoint first. Since this PR removesCartData
from checkout, that round-trip no longer occurs anywhere.However, we have a mechanism already that handles this sort of situation when checkout loads,
usePrepareProductsForCart
, which currently takes products from the URL and adds them to the cart. To that end, I think it makes more sense to read them from localStorage directly inside this hook. This PR does just that.Testing instructions
To test the
no-user
flow:/start/onboarding-registrationless/domains
I'm not sure how to test the
no-site
flow. Ping @Automattic/martech for help there.