theme-scripts icon indicating copy to clipboard operation
theme-scripts copied to clipboard

Theme Cart cancellable requests

Open liamdefty opened this issue 4 years ago • 8 comments

This library is proving super helpful to the team I'm working with, thanks! Just one thing we've ran into - currently there is no way to cancel a pending request. I propose an abort method is created that aborts any pending requests. We can achieve this using an Abort Controller.

The problem this solves are in this cases where a request is made that is no longer relevant, for example if we have a button to update an item in the cart that is pressed multiple times.

liamdefty avatar Oct 17 '19 23:10 liamdefty

Hey @liamdefty, thanks for the issue and the PR. Really cool - I wasn't aware of Abort Controller. I'm struggling to fully grasp your use case and whether this is something that should really be part of the cart package.

From what I gather, you could incorporate either one of two workarounds in the theme directly:

  1. Debounce the button functionality.
  2. Prevent the button from being clicked multiple times by disabling it while the request is being made.

Unless I'm mistaken - can you describe your scenario in more detail?

bertiful avatar Jun 09 '20 21:06 bertiful

Hey @chrisberthe

Thanks for replying! This was a while ago so let me try and jog my memory a little 🤔

Those two solutions are definitely viable options. However there are some issues with each of those.

  1. Debounce: It is possible to get into a scenario where we click the button, we wait the allocated time for debouncing and the request is sent. However, if we are on a slow connection, we may click the button again, the debounce time has passed and then a new request is sent... but the first request may not of finished yet. In the case of say, a plus/minus button in an Ajax cart this could cause issues with incorrect state.

  2. Preventing the button by disabling works, however can feel a little slow when using it. Again with the example for a plus/minus button on an Ajax cart we want to see the quantity update immediately and also be able to press it multiple times. For example to quickly increase the quantity from 1 to 5. Disabling the button means that we have to wait for each request to finish before being able to press again, which on a slow connection might take a while.

Cancelling the previous request works around these issues, I think 😅 . It is a little annoying that the fetch api doesn't let you handle this outside of where the fetch call is made, as I agree things like this is better handled in the theme but unfortunately this seems the only way when using fetch.

liamdefty avatar Jun 10 '20 09:06 liamdefty

@liamdefty wouldn't queueing requests be a more effective solution for those issues? You could try something like https://github.com/evan-liu/fetch-queue

Cam avatar Jun 11 '20 03:06 Cam

@Cam would this not mean there are more requests than needed? In the update example, it would be more efficient to send a single request with the final quantity instead of sending multiple separate requests to update the quantity by one that are queued?

Also cancelling a request is something built in to the fetch api, which means no need for external packages.

liamdefty avatar Jun 11 '20 08:06 liamdefty

@liamdefty In some ways yes. However when creating a queue you can add your own logic to remove or even merge specific requests when they become redundant.

As @chrisberthe mentioned, it's not something you would expect in a library that is intended as a wrapper for the Ajax cart API. That's something you would be expected to add as desired to a project.

Cam avatar Jun 12 '20 03:06 Cam

@cam Fair enough :)

Personally, I think having the option to cancel requests could be beneficial in a few other use cases and because of the way this is implemented using the fetch api there is no other way but to have it as part of the library.

Feel free to close this issue however if you disagree 👍

liamdefty avatar Jun 12 '20 13:06 liamdefty

@liamdefty I don't personally think it would be bad to add this. It's not like it adds much weight, especially being a native feature of the Fetch API. It certainly would make it super easy to add better queuing and merging of requests.

Honestly I'm seriously considering adding it to this project: https://shopify-theme.elkfox.io/modules/ajax-cart

EDIT: Oops! I forgot to finish what I was saying... While I don't personally think adding it would be a bad thing, I'm not sure it necessarily fits with the principles of this package. I was under the impression these modules are intended as fairly simple wrappers for the API that you would build on yourself on a case by case basis.

I could well be very wrong though!

Cam avatar Jun 13 '20 04:06 Cam

@Cam Apologies for the delayed reply - that makes sense! :)

liamdefty avatar Jun 22 '20 11:06 liamdefty