Automattic / wp-calypso

The JavaScript and API powered WordPress.com
https://developer.wordpress.com
GNU General Public License v2.0
12.43k stars 1.99k forks source link

getCartKey should return undefined when a no-site cart is not needed #84114

Open sirbrillig opened 1 year ago

sirbrillig commented 1 year ago

When the @automattic/shopping-cart package was refactored to support adding multiple carts (https://github.com/Automattic/wp-calypso/pull/54667), it meant that components which used the manager (usually via useShoppingCart or withShoppingCart) became responsible for knowing the cart key of the cart they wished to fetch. If the component is not ready to fetch the cart, it should provide the cart key undefined to the cart manager.

Most of the time, this key is the site ID of the selected site, which is stored in Redux state. And so I created getCartKey and its automated forms, useCartKey and withCartKey. These functions take the Redux state data and return the appropriate cart key for the selected site.

However, there are two special cart keys: no-user and no-site. These are used when no user exists or when no site exists, respectively and are used for several different flows. In order to return those values, getCartKey tried to replicate the existing logic for determining if the current context called for a userless or siteless cart, but unfortunately that logic was somewhat unclear. As a result, getCartKey will return no-site if it can't determine the correct cart key to use.

The result is that sometimes an unnecessary call to the shopping-cart endpoint is made with the no-site key when the consumer is not yet ready.

I hadn't considered this a major problem as the consumer component could ignore the result of getCartKey and pass undefined manually to the cart manager (when used with withShoppingCart this requires the use of its mapPropsToCartKey function), but in practice this never happens.

A better solution would be for getCartKey to return undefined when a siteless flow is not explicitly needed. We just need to make sure that no siteless flow is relying on the existing behavior.

michaeldcain commented 1 year ago

Moving to the backlog since it's more of an improvement than bug.