Skip to content
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.

Sign up for GitHub

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

Move Promise instantiation into memoized function to ensure caching consistency #9927

Merged
merged 13 commits into from
Dec 17, 2024
Merged

Move Promise instantiation into memoized function to ensure caching consistency #9927

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
66e78f7
move Promise instantiation into memoized function to ensure caching c…
Dec 11, 2024
93cc14e
add changelog entry
Dec 11, 2024
0710e79
Merge branch 'develop' into fix/unhandled-promises
timur27 Dec 12, 2024
e02ca10
Merge branch 'develop' into fix/unhandled-promises
timur27 Dec 12, 2024
90547b9
Merge branch 'develop' into fix/unhandled-promises
timur27 Dec 12, 2024
40358a3
Merge branch 'develop' into fix/unhandled-promises
timur27 Dec 12, 2024
d4dd4e2
Merge branch 'develop' into fix/unhandled-promises
timur27 Dec 13, 2024
2c3797e
Merge branch 'develop' into fix/unhandled-promises
timur27 Dec 13, 2024
0d29fe4
Merge branch 'develop' into fix/unhandled-promises
timur27 Dec 13, 2024
79853f6
Merge branch 'develop' into fix/unhandled-promises
timur27 Dec 13, 2024
8d07b8b
Merge branch 'develop' into fix/unhandled-promises
timur27 Dec 13, 2024
df0b214
Merge branch 'develop' into fix/unhandled-promises
timur27 Dec 16, 2024
89f30b7
Merge branch 'develop' into fix/unhandled-promises
timur27 Dec 17, 2024
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
4 changes: 4 additions & 0 deletions changelog/fix-unhandled-promises
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: minor
Type: fix

Fix payment method filtering when billing country changes in Blocks checkout.
8 changes: 2 additions & 6 deletions client/express-checkout/blocks/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,7 @@ const expressCheckoutElementApplePay = ( api ) => ( {
return false;
}

return new Promise( ( resolve ) => {
checkPaymentMethodIsAvailable( 'applePay', cart, resolve );
} );
return checkPaymentMethodIsAvailable( 'applePay', cart );
},
} );

Expand Down Expand Up @@ -77,9 +75,7 @@ const expressCheckoutElementGooglePay = ( api ) => {
return false;
}

return new Promise( ( resolve ) => {
checkPaymentMethodIsAvailable( 'googlePay', cart, resolve );
} );
return checkPaymentMethodIsAvailable( 'googlePay', cart );
},
};
};
Expand Down
122 changes: 63 additions & 59 deletions client/express-checkout/utils/checkPaymentMethodIsAvailable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,71 +14,75 @@ import WCPayAPI from 'wcpay/checkout/api';
import { getUPEConfig } from 'wcpay/utils/checkout';

export const checkPaymentMethodIsAvailable = memoize(
( paymentMethod, cart, resolve ) => {
// Create the DIV container on the fly
const containerEl = document.createElement( 'div' );
( paymentMethod, cart ) => {
return new Promise( ( resolve ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I added the memoize to avoid running the verification against Stripe multiple times. But if doing so means we might introduce bugs, maybe it’s best to remove it?

Although your fix works well according to my tests, I see that the function wrapped by the promise doesn't get executed again after updating the cart. This isn’t an issue for now since ECE isn't enabled/disabled dynamically (as I know of), but it could become a problem in the future.

I think we can leave it as you’ve proposed in this PR for now, as it seems to be working well.

Copy link
Contributor Author

@timur27 timur27 Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is a good point. I was looking at it but haven't included it into this PR. I think that caching itself is a pretty nice addition because otherwise, like you're mentioning, we will be making unnecessary third-party Stripe requests too often.

There is indeed a potential issue because at the moment, memoize uses the entire cart object for cache key generation, which doesn't work well for object comparison and could lead to improper cache hits/misses.

By making the cache key more specific with only the relevant fields that affect the payment method availability:

export const checkPaymentMethodIsAvailable = memoize(
	( paymentMethod, cart ) => {...},
	( paymentMethod, cart ) => {
		return JSON.stringify({
			paymentMethod,
			total_price: cart.cartTotals.total_price,
			country: cart.billingAddress?.country
		});
	}
);

we will ensure it runs on some specific updates. But for this, we'd need to know what fields are relevant in our context, e.g. if billing country change should trigger reviewing ece availability. I think we can open a follow up issue, if we consider this worth diving into further.

// Create the DIV container on the fly
const containerEl = document.createElement( 'div' );

// Ensure the element is hidden and doesn’t interfere with the page layout.
containerEl.style.display = 'none';
// Ensure the element is hidden and doesn’t interfere with the page layout.
containerEl.style.display = 'none';

document.querySelector( 'body' ).appendChild( containerEl );
document.querySelector( 'body' ).appendChild( containerEl );

const root = ReactDOM.createRoot( containerEl );
const root = ReactDOM.createRoot( containerEl );

const api = new WCPayAPI(
{
publishableKey: getUPEConfig( 'publishableKey' ),
accountId: getUPEConfig( 'accountId' ),
forceNetworkSavedCards: getUPEConfig(
'forceNetworkSavedCards'
),
locale: getUPEConfig( 'locale' ),
isStripeLinkEnabled: isLinkEnabled(
getUPEConfig( 'paymentMethodsConfig' )
),
},
request
);
const api = new WCPayAPI(
{
publishableKey: getUPEConfig( 'publishableKey' ),
accountId: getUPEConfig( 'accountId' ),
forceNetworkSavedCards: getUPEConfig(
'forceNetworkSavedCards'
),
locale: getUPEConfig( 'locale' ),
isStripeLinkEnabled: isLinkEnabled(
getUPEConfig( 'paymentMethodsConfig' )
),
},
request
);

root.render(
<Elements
stripe={ api.loadStripeForExpressCheckout() }
options={ {
mode: 'payment',
paymentMethodCreation: 'manual',
amount: Number( cart.cartTotals.total_price ),
currency: cart.cartTotals.currency_code.toLowerCase(),
} }
>
<ExpressCheckoutElement
onLoadError={ () => resolve( false ) }
root.render(
<Elements
stripe={ api.loadStripeForExpressCheckout() }
options={ {
paymentMethods: {
amazonPay: 'never',
applePay:
paymentMethod === 'applePay'
? 'always'
: 'never',
googlePay:
paymentMethod === 'googlePay'
? 'always'
: 'never',
link: 'never',
paypal: 'never',
},
mode: 'payment',
paymentMethodCreation: 'manual',
amount: Number( cart.cartTotals.total_price ),
currency: cart.cartTotals.currency_code.toLowerCase(),
} }
onReady={ ( event ) => {
let canMakePayment = false;
if ( event.availablePaymentMethods ) {
canMakePayment =
event.availablePaymentMethods[ paymentMethod ];
}
resolve( canMakePayment );
root.unmount();
containerEl.remove();
} }
/>
</Elements>
);
>
<ExpressCheckoutElement
onLoadError={ () => resolve( false ) }
options={ {
paymentMethods: {
amazonPay: 'never',
applePay:
paymentMethod === 'applePay'
? 'always'
: 'never',
googlePay:
paymentMethod === 'googlePay'
? 'always'
: 'never',
link: 'never',
paypal: 'never',
},
} }
onReady={ ( event ) => {
let canMakePayment = false;
if ( event.availablePaymentMethods ) {
canMakePayment =
event.availablePaymentMethods[
paymentMethod
];
}
resolve( canMakePayment );
root.unmount();
containerEl.remove();
} }
/>
</Elements>
);
} );
}
);
Loading