Automattic / woocommerce-payments

Accept payments via credit card. Manage transactions within WordPress.
https://wordpress.org/plugins/woocommerce-payments/
Other
175 stars 69 forks source link

Prevent enqueuing WCPay settings page JS on All payment methods page #2005

Closed waclawjacek closed 2 years ago

waclawjacek commented 3 years ago

When you go to WooCommerce > Settings > Payments > All payment methods and quickly navigate away from this page before it's finished loading, the Error retrieving settings. toast generated by scripts that should only be running on the WCPay settings page (loading data from the settings endpoint) is displayed.

The WCPay settings page scripts should not be enqueued on the "All payment methods" page.

waclawjacek commented 3 years ago

Okay, so I did some research and here is what I learned:

I have observed this issue on Firefox 88.0.1 but not on Chrome 91.0.4472.77.

The WCPAY_PAYMENT_GATEWAYS_PAGE script is enqueued on the All payment methods page. It calls useSettings() which makes an AJAX request to load settings from the back-end.

If you navigate away from this page while the AJAX request is in progress, an error saying Error retrieving settings. is displayed as a snackbar notification.

It seems that Firefox aborts XHR requests when you navigate to another page. When console.logging the error in the try-catch block, it is caused by this line.

This error can also be reproduced in other places - one that I tested is the disputes page. When you visit it and then navigate to a non-React page, you get a similar error saying Error retrieving disputes..

This probably isn't as much of an issue on pages where something is visibly loading as it is on the All payment methods page where everything seems to be loaded and this notification appears.

Markup on 2021-06-03 at 19:05:59

How to reproduce the XHR-aborting behavior:

  1. Go to https://wordpress.com so we don't run into a CORS issue.
  2. Paste this into the dev tools console:
    • fetch( 'https://wordpress.com' ).then( res => { console.log( 'Request succeeded! 🎉' ) } ).catch( err => { console.log( 'Request failed! 😭)' ) } );
    • You should get Request succeeded! 🎉
  3. Now, paste this into the dev tools console (same thing but simulating navigating away right after making the request):
    • fetch( 'https://wordpress.com' ).then( res => { console.log( 'Request succeeded! 🎉' ) } ).catch( err => { console.log( 'Request failed! 😭)' ) } ); window.location = 'https://wordpress.com'
    • You should get Request failed! 😭

Some possible solutions include:

  1. Somehow detecting if the user is navigating away from the page or has manually aborted the request.
  2. Silencing the error in some cases.
  3. Not using useSettings() on the All payment methods page and using something else instead.
frosso commented 3 years ago

Thanks @waclawjacek !

I took a quick look and I couldn’t get access to the raw request of the apiFetch generator. If we had access to the raw request, we could determine the kind of error code, but unfortunately the exception thrown just says fetch_error with a You are probably offline. error message - not super useful for our use case. If we could determine the kind of error, I think we could be in great shape.

In my mind I think that option #3 might be the easiest one to implement, without having to incur in additional branching logic, maybe? The page only needs access to the enabled payment method ids (at the moment). It looks like those elements could be passed from the backend via wp_localize_script (instead of using useSettings).

The same error message could also appear on the settings or onboarding pages.

I pulled it to the current sprint, but with lower priority (given it might also be browser-specific) - I wouldn't want other items that are already in the sprint to be deprioritized :)

gpressutto5 commented 2 years ago

I could confirm this error happens only in Firefox. Chrome and Safari are ok. I found this Bug report, and it looks like a WONTFIX: https://bugzilla.mozilla.org/show_bug.cgi?id=1280189

Things I've tried:

I will keep looking for another solution.

gpressutto5 commented 2 years ago

After discussing with @waclawjacek, we decided it is better to leave it as it is for now because it happens only on firefox, they are aware of that, and it seems like nobody has a good solution for that until Mozilla fixes that or gives us resources to handle that.