Wait for Stripe js to load before using it #9770
Conversation
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. |
|
Size Change: +677 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
danielmx-dev
requested review from
a team and
mdmoore
and removed request for
a team
There was a problem hiding this comment.
This was testing well with the initial test cases. When I attempted to test Pay for Order, I noticed the credit card form wasn't loading. Testing further while logged in, it seems Stripe JS isn't being loaded other pages like Checkout and Cart either. This is likely related to the the hardcoded changes in the Cookiebot plugin combined with the fact that the banner doesn't appear when logged in (so cookies can't be accepted), but I want to double check with you on this and whether you've been able to test while logged in?
I assume it may be related to this setting ?
However, I'm not able to replicate the issue with the setting On or Off. I would assume that if the banner doesn't show up then the script shouldn't be blocked. |
There was a problem hiding this comment.
I assume it may be related to this setting ? Cookiebot CMP on front-end while logged in
Yeah that did the trick for displaying the banner. With that setting off, Stripe.js never loads for logged in users in my testing. I know you're unable to reproduce it, so maybe it's something with my settings. Do you think this is something to be concerned about? If not, this has my approval.
What happens if you test the faulty scenario on develop? Although the behavior of the Cookiebot is concerning, I don't think the changes in this PR make the situation worse, if another plugin is just blocking our dependencies there's little we can do about it. |
* develop: Wait for Stripe js to load before using it (#9770) Remove redundant stripe payment elements mount for pay for order (#9813) Enforce proper return types for methods get_order_from_event_body (#9761) Update phpcompatibility to develop version to get sniffs for PHP 8 (#9697) Enable ECE Tracks Events when WooPay is disabled (#9793) chore: rename PRB constants to ECE (#9768) Amend changelog entries for release 8.5.1 fix: remove 'test mode' badge from shortcode checkout (#9800) Update version and add changelog entries for release 8.5.1
| ), | ||
| } ); | ||
| } | ||
| const cartData = await getCartApiHandler().getCart(); |
There was a problem hiding this comment.
This change introduced a difference in how errors are handled.
Before the change, any errors by getCart() weren't propagated, and they could have just generated a warning in the console.
Now, there is no try/catch, so any errors from getCart() are being propagated and will lead to a termination of the execution.
@danielmx-dev let me know your thoughts - I reverted this in #9849
| async getStripe( forceAccountRequest = false ) { | ||
| const maxWaitTime = 600 * 1000; // 600 seconds | ||
| const waitInterval = 100; | ||
| let currentWaitTime = 0; | ||
| while ( ! window.Stripe ) { | ||
| await new Promise( ( resolve ) => | ||
| setTimeout( resolve, waitInterval ) | ||
| ); | ||
| currentWaitTime += waitInterval; | ||
| if ( currentWaitTime > maxWaitTime ) { | ||
| throw new Error( 'Stripe object not found' ); | ||
| } | ||
| } | ||
| return this.__getStripe( forceAccountRequest ); | ||
| } |
There was a problem hiding this comment.
While commenting on the try/catch below, I noticed this implementation: I was wondering if you had considered leveraging JavaScript Proxies to detect when Stripe is added to the window object, rather than using a timeout in a loop.
I was also wondering if you had considered raising the error in a more user-friendly fashion, like in the payment method box, to allow the customer to review the cookies consent.
Fixes #9709
Changes proposed in this Pull Request
WCPayAPI#getStripemethod from synchronous to asynchronous. This allow us to wait for Stripe.js to load in scenarios where the script download may be delayed due to third party plugins that ask for user's consent (see details in Handling of Stripe JS with GDPR & minifier plugins #9709).Testing instructions
Script_Loader_Tag::__constructin the plugin code to always set thescript_loader_tagfilter.Cookiebot_Addons::build_containerin the plugin code:$this->container->get( 'Script_Loader_Tag_Interface' )->add_tag( 'stripe', array( 'marketing', 'statistics' ) );Before
ReferenceError: Stripe is not defineddocument.cookie = 'CookieConsent=; Path=/;';After:
Regression Testing
Disable Cookiebot and verify that the same pages (Blocks/Classic Checkout, Cart (Blocks and Classic), Product Pages.) continue working as expected.
npm 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