Move Promise instantiation into memoized function to ensure caching consistency #9927
+69
−65
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +10 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
timur27
requested review from
a team and
mdmoore
and removed request for
a team
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
asumaran
approved these changes
Dec 12, 2024
There was a problem hiding this comment.
LGTM.
- Tested with multiple payment methods enabled and can confirm they toggle based on the country. Also tested with the euro currency to verify euro-based payment methods.
- Tested that payment methods are hidden after applying a 100% discount coupon.
- Tested and confirmed that the fix for woocommerce/woocommerce#50353 is still applied.
| // Create the DIV container on the fly | ||
| const containerEl = document.createElement( 'div' ); | ||
| ( paymentMethod, cart ) => { | ||
| return new Promise( ( resolve ) => { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #9851
Changes proposed in this Pull Request
It was discovered that for some reason,
canMakePaymentfor payment methods (non-express checkout payment methods) is executed only upon page load, but not executed on subsequent actions like e.g. billing country update. This applies to Blocks checkout only.The side effect ended up to be in the
canMakePayment's callback implementation for express checkout methods. There are two aspects worth mentioning:1️⃣ WooCommerce Blocks takes all the registered payment methods and then calls
canMakePaymenton each of those. Typically, express checkout payment methods are evaluated first due to the default registration order, unless explicitly configured otherwise.2️⃣ The
canMakePayment's callback implementation for express checkout methods usesmemoize, which caches the resolved result (a boolean) from the first execution. Subsequent calls to this function return the cached boolean instead of a promise with some resolution state, leaving promises unresolved in those cases. This behavior caused the promises to remain unhandled, as the cache bypassed the promise resolution logic.This PR resolves the issue by caching the promise itself instead of its resolved value. Previously, memoize cached the resolved boolean, causing subsequent calls to bypass the promise logic. Now, the memoized function returns and caches the promise, ensuring that subsequent calls return the cached resolved promise which is handled by Blocks.
Testing instructions
For Blocks checkout
developthat dynamical filtering upon billing country change is not workingfix/unhandled-promisesand make sure that dynamical filtering of payment methods worksnpm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge