improve responsiveness when setting cart item quantities #1864

haszari · 2020-03-04T02:55:10Z

This PR improves how cart line item quantities work, allowing the user to increase/decrease quantity, and defers the server update to reduce unnecessary network requests.

The generator action for persisting quantity (changeCartItemQuantity) is now simpler. It doesn't set the item as pending, so the UI stays enabled and available when API requests are in progress.
Added local state in CartLineItemRow component so the quantity in the UI updates immediately.
The UI quantity value is debounced and then persisted when stable (via changeCartItemQuantity).

How to test the changes in this Pull Request:

Add cart block to a page and publish, add some stuff to cart.
Change item quantity using quantity picker - plus/minus buttons and text edit.
Confirm that quantities correctly persist and the UI is responsive to user input at all times.

haszari · 2020-03-05T02:54:42Z

Note: the E2E test failure is not due to this branch, this is covered by #1869.

senadir

overall this is looking good, and it tests good, some points that could be cleaned before merging

assets/js/blocks/cart-checkout/cart/full-cart/cart-line-item-row.js

senadir · 2020-03-05T19:50:30Z

assets/js/blocks/cart-checkout/cart/full-cart/cart-line-item-row.js

@@ -68,9 +83,9 @@ const CartLineItemRow = ( { lineItem = {} } ) => {
 			<td className="wc-block-cart-item__quantity">
 				<QuantitySelector
 					disabled={ itemQuantityDisabled }


and this, also other occurrence of disable

senadir · 2020-03-05T20:36:31Z

assets/js/blocks/cart-checkout/cart/full-cart/cart-line-item-row.js

@@ -43,6 +44,20 @@ const CartLineItemRow = ( { lineItem = {} } ) => {
 		isPending: itemQuantityDisabled,
 	} = useStoreCartItem( key );

+	// Store item quantity in local state so the UI can update independently
+	// of store/server updates.
+	const [ localQuantity, updateLocalQuantity ] = useState( quantity );


any reason why other components are not using localQuantity instead of quantity it should make other updates immediate (like saving and item total), the server can later update.

Good point, I'll look at that :)

Added this as part of the recent changes, definitely makes it better seeing the discount/totals update live.

senadir · 2020-03-05T20:48:44Z

assets/js/blocks/cart-checkout/cart/full-cart/cart-line-item-row.js

+	useEffect( () => {
+		if ( key ) {
+			changeQuantity( debouncedQuantity );
+		}
+	}, [ key, debouncedQuantity ] );


for now, this triggers an update on initial render, we don’t need that, also there is no need to check for key and watch it since it’s stable and required for this component to work, so we can update this to avoid extra render+call for each item.

Suggested change

useEffect( () => {

if ( key ) {

changeQuantity( debouncedQuantity );

}

}, [ key, debouncedQuantity ] );

useEffect( () => {

if ( quantity !== debouncedQuantity ) {

changeQuantity( debouncedQuantity );

}

}, [ debouncedQuantity ] );

This was needed to prevent an issue in early renders - component rendering with no/invalid state. But perhaps that's an issue higher up, I'll investigate.

Hmm, looks like this was a remnant/workaround from a different iteration. I've fixed this here 960357c (of course may change if I refactor the state/debounce).

haszari · 2020-03-05T23:05:44Z

Rebased to fix conflicts.

haszari · 2020-03-06T01:27:52Z

When testing this I saw an issue - shopper can increase quantity past the stock level. #1887

When we fix that it may impact some of this - e.g. if the debounced API request returns an error we need to overwrite the optimistic changes in the UI and show a notice/message.

haszari · 2020-03-06T02:47:10Z

assets/js/base/hooks/use-store-cart-item.js

-		( select, { dispatch } ) => {
+	// Store quantity in hook state. This is used to keep the UI
+	// updated while server request is updated.
+	const [ quantity, changeQuantity ] = useState( cartItem?.quantity );


I had to add the careful cartItem?.quantity optional chain operator here because this hook fires in the editor, causing JS error. I think this is because the block is using preview data but the hook still loads real cart data.

I think we need to fix this issue (as follow up) - the cart block shouldn't pull real data in the editor at all. Not sure of the best way to do this (e.g. shouldSelect, isEditor props, or maybe we can set "isEditor" in context somewhere so hooks can transparently support this. cc @nerrad

Ya this is because cartItems could change. If you implement my suggestion for deriving cartItem via useEffect then that should take care of this issue.

haszari · 2020-03-06T02:56:08Z

This is ready for review again @senadir @nerrad.

I've now moved the debounce to the hook and it feels quite neat and tidy this way. The part that was confusing me is that updating quantity on the server is a side effect of updating the component / hook, which feels strange – but maybe a tiny bit less strange when it's not hidden in a hook. I'm on the fence about how closely this hook is "coupled" to cart line item component, but we can keep an eye on that as we iterate.

I worked around an issue with the hook firing in the editor – I haven't logged a follow up issue for that yet. See comment.

senadir · 2020-03-06T09:51:07Z

assets/js/base/hooks/use-store-cart-item.js

 export const useStoreCartItem = ( cartItemKey ) => {
 	const { cartItems, cartIsLoading } = useStoreCart();
-	const cartItem = cartItems.filter( ( item ) => item.key === cartItemKey );
+	const cartItem = cartItems.find( ( item ) => item.key === cartItemKey );


we can avoid this by simply passing the cartItems to the cart, this should allow us to reload the data again (since we initially loaded it anyway), however, I'm not fully sure this is the best approach (do we want the hook to get the full item, or simply they key, I think it should be either not passing anything or passing everything).

Suggested change

export const useStoreCartItem = ( cartItemKey ) => {

const { cartItems, cartIsLoading } = useStoreCart();

const cartItem = cartItems.filter( ( item ) => item.key === cartItemKey );

const cartItem = cartItems.find( ( item ) => item.key === cartItemKey );

export const useStoreCartItem = ( cartItem ) => {

we can avoid this by simply passing the cartItems to the cart, this should allow us to reload the data again (since we initially loaded it anyway)

This sounds a bit confusing initially, but I think what you're suggesting here is to avoid an unnecessary request triggered by implementing useStoreCart, you have useStoreCartItem receive the cartItem directly and then it doesn't need to implement useStoreCart (as evidenced from your example).

However, the primary benefit of this hook is to derive the cart item from a given cart item key and return it (so component code doesn't have to have that logic).

useStoreCart can be modified so that it reads the context from something like useCheckoutContext (as per this pull being evaluated) and that determines whether it returns a default preview cart object or not. So in my opinion, useStoreCart should be making the determination on whether requests are made.

To be clear, this isn't a blocking discussion for this pull.

nerrad

Sorry Rua, the comments here are something I missed in my original review. Hope the suggestions help :)

nerrad · 2020-03-06T15:51:52Z

assets/js/base/hooks/use-store-cart-item.js

@@ -22,32 +24,43 @@ import { useStoreCart } from './use-store-cart';
 */
 export const useStoreCartItem = ( cartItemKey ) => {
 	const { cartItems, cartIsLoading } = useStoreCart();
-	const cartItem = cartItems.filter( ( item ) => item.key === cartItemKey );
+	const cartItem = cartItems.find( ( item ) => item.key === cartItemKey );


I just realized, deriving the cartItem should probably be done in an effect because cartItems might change (incidentally, this is what can cause the issue with the quantity state).

Suggested change

const cartItem = cartItems.find( ( item ) => item.key === cartItemKey );

// note for example I'm just doing `{ quantity: 0, key: '' }` here,

// but this should actually be a default object in the shape of

// a cartItem schema

const [ cartItem, setCartItem ] = useState( { quantity: 0, key: '' } );

useEffect( () => {

if ( ! cartIsLoading && cartItem.key !== cartItemKey ) {

const cartItem = cartItems.find( ( item ) => item.key === cartItemKey );

if ( cartItem ) {

setCartItem( cartItem );

}

}

}, [ cartItems, cartIsLoading, cartItem );

I tried to get this working, but the default quantity values in the state cartItem ended up resetting the real cart item quantity to zero on load, and persistence wasn't working at all.

I also tried a variation where the hook only provides quantity state (instead of the full cartItem). It didn't feel right having the full cart object in state, especially since the client component doesn't actually use it. However that had the same issue - the default quantity overwrote the real quantity.

So - @nerrad I'm thinking we merge this PR as is since it's testing and working well. We can still of course iterate on how this hook works, but I'm hesitant to get bogged down in this. Or if you want to jump on the PR and implement this please go ahead :)

Note to be clear the optional chain operator is only needed in the editor. I think what's happening is the preview data (no real/ajax data from hooks) is used, and then in CartLineItemRow the hook fires which requests real cart items. I see this as a bug – we should prevent this from making any real cart AJAX requests in the editor, that's the real cause of the issue IMO.

I accepted the invitation to address this and I think what I've pushed should work. My bad because yes the original suggested code was not quite right.

See if this works well for you and if it does, let's merge.

nerrad · 2020-03-06T15:53:26Z

assets/js/base/hooks/use-store-cart-item.js

-		( select, { dispatch } ) => {
+	// Store quantity in hook state. This is used to keep the UI
+	// updated while server request is updated.
+	const [ quantity, changeQuantity ] = useState( cartItem?.quantity );


Ya this is because cartItems could change. If you implement my suggestion for deriving cartItem via useEffect then that should take care of this issue.

- work in progress - still need to figure out how to debounce API call - add new action for updating quantity for an item - don't set cart item as pending while quantity is updating - this leaves QuantitySelector enabled so user can click more/less - use receiveCartItemQuantity to update quantity in UI before sending request

- use local state for quantity, so ui allows multiple clicks up/down - debounce store updates (and server/API call)

- reduces unnecessary renders

…adir

…adir)

+ fix latent bug in useStoreCartItem - the cartItem value is now object: - was previously single-item array - (note no client code is using this at present)

nerrad · 2020-03-08T23:13:22Z

assets/js/base/hooks/use-store-cart-item.js

+		if ( debouncedQuantity === 0 ) {
+			changeQuantity( cartItem.quantity );
+			return;
+		}


This covers initializing things when debouncedQuantity is 0 we want to make sure that the quantities update when we've found a cartItem.

This causes an extra POST request for each cart item on page load, redundantly re-setting the quantity. Are we ok with that?

To close the loop, we definitely did not want an extra POST request. But the issue was that the action (below these lines) was posting even if the quantity was the same as what was in the state. So I fixed this by ensuring the action only pinged the server to update the quantity if needed (by checking the current quantity in the state to make sure it didn't match).

nerrad · 2020-03-08T23:14:48Z

assets/js/base/hooks/use-store-cart-item.js

+			changeQuantity( cartItem.quantity );
+			return;
+		}
+		changeCartItemQuantity( cartItemKey, debouncedQuantity );


You don't need to compare to the quantity here because the effect will only fire when any of the dependencies change (if debouncedQuantity is the same then there's no worry about updating things because the state won't change.

nerrad · 2020-03-08T23:17:02Z

assets/js/base/hooks/use-store-cart-item.js

+	useEffect( () => {
+		if ( ! cartIsLoading ) {
+			const foundCartItem = cartItems.find(
+				( item ) => item.key === cartItemKey
+			);
+			if ( foundCartItem ) {
+				setCartItem( foundCartItem );
+			}
+		}
+	}, [ cartItems, cartIsLoading, cartItemKey ] );


We need to update cartItem every time cartItems change because the quantities will be different in the state. So basically whenever debouncedQuantity triggers an update to the state and cartItems change, then the internal cartItem here will change as well.

haszari · 2020-03-09T00:05:58Z

assets/js/base/hooks/use-store-cart-item.js

+		quantity: 0,
+		isPending: false,
+		changeQuantity: () => void null,
+		removeItem: () => void null,


How will we keep this cartItem defaults up to date? This is replicated in a few places now. If we iterate on this schema in future, will we need to manually keep this up to date or will we be guided there by bugs or tooling?

We could add a typedef for this, then tooling would warn us about JS code that's out of date with the schema (as long as we update the type!).

I think this default schema is actually wrong here because it's mixing cartItem with the object returned from the hook StoreCartItem. So I'll need to update this.

With regards to the default shape, a typedef would definitely help here but only if cartItem is exposed outside of this hook. I'll fix this default and consider typedef in a followup.

haszari · 2020-03-09T00:07:59Z

assets/js/base/hooks/use-store-cart-item.js

+		key: '',
+		isLoading: true,
+		cartData: {},
+		quantity: 0,


This 0 is a special value that we use for forcing an update during load. We should use a const instead of literal, and we could even use a more distinct placeholder value (e.g. -1).

haszari · 2020-03-09T00:09:32Z

assets/js/base/hooks/use-store-cart-item.js

+		isPending,
+		quantity,
+		changeQuantity,
+		removeItem,
 		isLoading: cartIsLoading,
 		cartItem,


Note that the client code doesn't actually use cartItem .. perhaps we don't need to return it. Pondering this - this hook is mainly about cart item "metadata" - isPending, quantity, and actions for remove/change quantity. The actual cart item data is accessed via other hooks.

Ya I had thought the same thing actually. I'll remove in followup.

haszari changed the title ~~improve responsiveness when setting cart item quantities (in progress)~~ improve responsiveness when setting cart item quantities Mar 5, 2020

haszari self-assigned this Mar 5, 2020

haszari added skip-changelog PRs that you don't want to appear in the changelog. status: needs review labels Mar 5, 2020

haszari added this to the Future Release milestone Mar 5, 2020

haszari marked this pull request as ready for review March 5, 2020 02:52

haszari requested a review from a team as a code owner March 5, 2020 02:52

haszari requested review from Aljullu, senadir and nerrad and removed request for a team March 5, 2020 02:52

senadir force-pushed the fix/debouce-quantity-changes branch from 3f1fd68 to 673a004 Compare March 5, 2020 17:45

senadir approved these changes Mar 5, 2020

View reviewed changes

woocommercebot added status: ready to merge and removed status: needs review labels Mar 5, 2020

senadir reviewed Mar 5, 2020

View reviewed changes

haszari force-pushed the fix/debouce-quantity-changes branch from 673a004 to 8ef90c9 Compare March 5, 2020 23:05

haszari force-pushed the fix/debouce-quantity-changes branch from d0bf580 to 621643b Compare March 6, 2020 01:12

haszari mentioned this pull request Mar 6, 2020

Don't allow shopper to increase quantity past stock max #1887

Closed

haszari commented Mar 6, 2020

View reviewed changes

senadir reviewed Mar 6, 2020

View reviewed changes

nerrad reviewed Mar 6, 2020

View reviewed changes

haszari added 4 commits March 9, 2020 08:43

debounce line item quantity first cut:

77cec7f

- use local state for quantity, so ui allows multiple clicks up/down - debounce store updates (and server/API call)

correct comment on cart item quantity reducer

ecc2c71

remove recieveCartItemQuantity - no longer needed

dcc3fef

haszari added 8 commits March 9, 2020 08:43

remove delegation for deleted RECEIVE_CART_ITEM_QUANTITY

f29b16e

only update quantity in component sideffect if it has changed:

c2a71df

- reduces unnecessary renders

factor out debounced quantity update into cartItem hook (hat tip @sen…

7bf1bc4

…adir)

use quantity from store, instead of passing in to hook +

50808d0

+ fix latent bug in useStoreCartItem - the cartItem value is now object: - was previously single-item array - (note no client code is using this at present)

tidy/refactor cart item hook - separate dispatch from select

1650093

remove dud reset of item pending flag (came back in rebase)

b1296db

add quantity to StoreCartItem hook return value typedef

e11c91d

fix js error when adding cart block in editor – cartItem not found

6da32b5

haszari force-pushed the fix/debouce-quantity-changes branch from d9de14a to 6da32b5 Compare March 8, 2020 19:43

nerrad added 2 commits March 8, 2020 19:06

fix typedef

1558e41

fix logic for debouncing

6e6f028

nerrad reviewed Mar 8, 2020

View reviewed changes

haszari commented Mar 9, 2020

View reviewed changes

don’t update quantity on server unnecessarily

077b233

haszari merged commit ff91a98 into master Mar 9, 2020

haszari deleted the fix/debouce-quantity-changes branch March 9, 2020 02:09

nerrad mentioned this pull request Mar 9, 2020

Followup for useStoreCartItem work (change hook name and improve typedefs) #1900

Merged

nerrad modified the milestones: Future Release, 2.6.0 Apr 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve responsiveness when setting cart item quantities #1864

improve responsiveness when setting cart item quantities #1864

haszari commented Mar 4, 2020 •

edited

Loading

haszari commented Mar 5, 2020

senadir left a comment

senadir Mar 5, 2020

senadir Mar 5, 2020

haszari Mar 5, 2020

haszari Mar 6, 2020

senadir Mar 5, 2020

haszari Mar 5, 2020

haszari Mar 5, 2020

haszari commented Mar 5, 2020

haszari commented Mar 6, 2020

haszari Mar 6, 2020

nerrad Mar 6, 2020

haszari commented Mar 6, 2020

senadir Mar 6, 2020

nerrad Mar 6, 2020

nerrad left a comment

nerrad Mar 6, 2020

haszari Mar 8, 2020

nerrad Mar 8, 2020

nerrad Mar 6, 2020

nerrad Mar 8, 2020

haszari Mar 9, 2020

nerrad Mar 9, 2020

nerrad Mar 8, 2020

nerrad Mar 8, 2020

haszari Mar 9, 2020

haszari Mar 9, 2020

nerrad Mar 9, 2020

haszari Mar 9, 2020

haszari Mar 9, 2020

nerrad Mar 9, 2020

-	const cartItem = cartItems.find( ( item ) => item.key === cartItemKey );
+    // note for example I'm just doing `{ quantity: 0, key: '' }` here,
+    // but this should actually be a default object in the shape of
+    //  a cartItem schema
+    const [ cartItem, setCartItem ] = useState( { quantity: 0, key: '' } );
+    useEffect( () => {
+        if ( ! cartIsLoading && cartItem.key !== cartItemKey ) {
+            	const cartItem = cartItems.find( ( item ) => item.key === cartItemKey );
+            	if ( cartItem ) {
+            	    setCartItem( cartItem );
+            	}
+        }
+    }, [ cartItems, cartIsLoading, cartItem );

improve responsiveness when setting cart item quantities #1864

improve responsiveness when setting cart item quantities #1864

Conversation

haszari commented Mar 4, 2020 • edited Loading

How to test the changes in this Pull Request:

haszari commented Mar 5, 2020

senadir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haszari commented Mar 5, 2020

haszari commented Mar 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haszari commented Mar 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nerrad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haszari commented Mar 4, 2020 •

edited

Loading