Skip to content
This repository has been archived by the owner on Feb 23, 2024. It is now read-only.

Refactor external dispatch actions from being called inside useSelect #6718

Merged
merged 6 commits into from
Jul 29, 2022
Merged

Refactor external dispatch actions from being called inside useSelect #6718

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f776ee8
refactor coupon functions outside of useSelect
senadir Jul 19, 2022
c40fe94
Merge branch 'trunk' into refactor/remove-notice-calls-from-select
senadir Jul 27, 2022
6dbfc5f
fix test
senadir Jul 27, 2022
812160c
Merge branch 'trunk' into refactor/remove-notice-calls-from-select
senadir Jul 27, 2022
44be973
Merge branch 'trunk' into refactor/remove-notice-calls-from-select
senadir Jul 29, 2022
61116bb
Merge branch 'trunk' into refactor/remove-notice-calls-from-select
senadir Jul 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions assets/js/atomic/blocks/product-elements/image/test/block.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,6 @@ import { ProductDataContextProvider } from '@woocommerce/shared-context';
*/
import { Block } from '../block';

jest.mock( '@woocommerce/block-settings', () => ( {
...jest.requireActual( '@woocommerce/block-settings' ),
__esModule: true,
PLACEHOLDER_IMG_SRC: 'placeholder.jpg',
} ) );

jest.mock( '../../../../../hooks/style-attributes', () => ( {
__esModule: true,
useBorderProps: jest.fn( () => ( {
Expand Down
171 changes: 87 additions & 84 deletions assets/js/base/context/hooks/cart/use-store-cart-coupons.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,103 +29,106 @@ export const useStoreCartCoupons = ( context = '' ): StoreCartCoupon => {
const { createNotice } = useDispatch( 'core/notices' );
const { setValidationErrors } = useValidationContext();

const results: Pick<
const {
applyCoupon,
removeCoupon,
isApplyingCoupon,
isRemovingCoupon,
}: Pick<
StoreCartCoupon,
'applyCoupon' | 'removeCoupon' | 'isApplyingCoupon' | 'isRemovingCoupon'
| 'applyCoupon'
| 'removeCoupon'
| 'isApplyingCoupon'
| 'isRemovingCoupon'
| 'receiveApplyingCoupon'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a broken TS, wasn't sure what's the best fix @opr either add receiveApplyingCoupon to StoreCartCoupon or have another sort of union. Not even sure what StoreCartCoupon should be and what should it include.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to type this, it should be inferred, and it also seems weird to still pick dispatch actions inside useSelect I propose we drop the explicit typing, remove the actions from the useSelect and get them from useDispatch instead. This is consistent with how it's being done elsewhere in the codebase and the data store refactor work.

I tested this on wpcom and it works still.

See this example patch and let me know what you think.

From c7787796b798e7914f290c03f484b9fd19f99a7f Mon Sep 17 00:00:00 2001
From: Thomas Roberts <thomas.roberts@automattic.com>
Date: Fri, 22 Jul 2022 09:35:31 +0100
Subject: [PATCH] Add new dispatch outside of select

---
 .../hooks/cart/use-store-cart-coupons.ts      | 23 ++++---------------
 1 file changed, 5 insertions(+), 18 deletions(-)

diff --git a/assets/js/base/context/hooks/cart/use-store-cart-coupons.ts b/assets/js/base/context/hooks/cart/use-store-cart-coupons.ts
index aa5cd0bf8..2115fd330 100644
--- a/assets/js/base/context/hooks/cart/use-store-cart-coupons.ts
+++ b/assets/js/base/context/hooks/cart/use-store-cart-coupons.ts
@@ -29,34 +29,21 @@ export const useStoreCartCoupons = ( context = '' ): StoreCartCoupon => {
 	const { createNotice } = useDispatch( 'core/notices' );
 	const { setValidationErrors } = useValidationContext();
 
-	const {
-		applyCoupon,
-		removeCoupon,
-		isApplyingCoupon,
-		isRemovingCoupon,
-	}: Pick<
-		StoreCartCoupon,
-		| 'applyCoupon'
-		| 'removeCoupon'
-		| 'isApplyingCoupon'
-		| 'isRemovingCoupon'
-		| 'receiveApplyingCoupon'
-	> = useSelect(
-		( select, { dispatch } ) => {
+	const { isApplyingCoupon, isRemovingCoupon } = useSelect(
+		( select ) => {
 			const store = select( storeKey );
-			const actions = dispatch( storeKey );
 
 			return {
-				applyCoupon: actions.applyCoupon,
-				removeCoupon: actions.removeCoupon,
 				isApplyingCoupon: store.isApplyingCoupon(),
 				isRemovingCoupon: store.isRemovingCoupon(),
-				receiveApplyingCoupon: actions.receiveApplyingCoupon,
 			};
 		},
 		[ createErrorNotice, createNotice ]
 	);
 
+	const { applyCoupon, removeCoupon, receiveApplyingCoupon } =
+		useDispatch( storeKey );
+
 	const applyCouponWithNotices = ( couponCode: string ) => {
 		applyCoupon( couponCode )
 			.then( ( result ) => {
-- 
2.32.1 (Apple Git-133)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Thomas! I applied the changes.

> = useSelect(
( select, { dispatch } ) => {
const store = select( storeKey );
const isApplyingCoupon = store.isApplyingCoupon();
const isRemovingCoupon = store.isRemovingCoupon();
const {
applyCoupon,
removeCoupon,
receiveApplyingCoupon,
}: {
applyCoupon: ( coupon: string ) => Promise< boolean >;
removeCoupon: ( coupon: string ) => Promise< boolean >;
receiveApplyingCoupon: ( coupon: string ) => void;
} = dispatch( storeKey );

const applyCouponWithNotices = ( couponCode: string ) => {
applyCoupon( couponCode )
.then( ( result ) => {
if ( result === true ) {
createNotice(
'info',
sprintf(
/* translators: %s coupon code. */
__(
'Coupon code "%s" has been applied to your cart.',
'woo-gutenberg-products-block'
),
couponCode
),
{
id: 'coupon-form',
type: 'snackbar',
context,
}
);
}
} )
.catch( ( error ) => {
setValidationErrors( {
coupon: {
message: decodeEntities( error.message ),
hidden: false,
},
} );
// Finished handling the coupon.
receiveApplyingCoupon( '' );
} );
};

const removeCouponWithNotices = ( couponCode: string ) => {
removeCoupon( couponCode )
.then( ( result ) => {
if ( result === true ) {
createNotice(
'info',
sprintf(
/* translators: %s coupon code. */
__(
'Coupon code "%s" has been removed from your cart.',
'woo-gutenberg-products-block'
),
couponCode
),
{
id: 'coupon-form',
type: 'snackbar',
context,
}
);
}
} )
.catch( ( error ) => {
createErrorNotice( error.message, {
id: 'coupon-form',
context,
} );
// Finished handling the coupon.
receiveApplyingCoupon( '' );
} );
};
const actions = dispatch( storeKey );

return {
applyCoupon: applyCouponWithNotices,
removeCoupon: removeCouponWithNotices,
isApplyingCoupon,
isRemovingCoupon,
applyCoupon: actions.applyCoupon,
removeCoupon: actions.removeCoupon,
Comment on lines +50 to +51
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could just move this out of the useSelect callback too? (same with receiveApplyingCoupon)

isApplyingCoupon: store.isApplyingCoupon(),
isRemovingCoupon: store.isRemovingCoupon(),
receiveApplyingCoupon: actions.receiveApplyingCoupon,
};
},
[ createErrorNotice, createNotice ]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These dependencies shouldn't be needed now?

);

const applyCouponWithNotices = ( couponCode: string ) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how often createNotice or setValidationErrors or createErrorNotice change (can't remember if they themselves are dispatch function) but one optimization here might be to memoize the new composite dispatchers via useCallback. I think that might have been why they were in the useSelect callback originally so that we memoized them based on the dependencies.

applyCoupon( couponCode )
.then( ( result ) => {
if ( result === true ) {
createNotice(
'info',
sprintf(
/* translators: %s coupon code. */
__(
'Coupon code "%s" has been applied to your cart.',
'woo-gutenberg-products-block'
),
couponCode
),
{
id: 'coupon-form',
type: 'snackbar',
context,
}
);
}
} )
.catch( ( error ) => {
setValidationErrors( {
coupon: {
message: decodeEntities( error.message ),
hidden: false,
},
} );
// Finished handling the coupon.
receiveApplyingCoupon( '' );
} );
};

const removeCouponWithNotices = ( couponCode: string ) => {
removeCoupon( couponCode )
.then( ( result ) => {
if ( result === true ) {
createNotice(
'info',
sprintf(
/* translators: %s coupon code. */
__(
'Coupon code "%s" has been removed from your cart.',
'woo-gutenberg-products-block'
),
couponCode
),
{
id: 'coupon-form',
type: 'snackbar',
context,
}
);
}
} )
.catch( ( error ) => {
createErrorNotice( error.message, {
id: 'coupon-form',
context,
} );
// Finished handling the coupon.
receiveApplyingCoupon( '' );
} );
};

return {
appliedCoupons: cartCoupons,
isLoading: cartIsLoading,
...results,
applyCoupon: applyCouponWithNotices,
removeCoupon: removeCouponWithNotices,
isApplyingCoupon,
isRemovingCoupon,
};
};