swup / swup

Versatile and extensible page transition library for server-rendered websites 🎉
https://swup.js.org
MIT License
4.63k stars 195 forks source link

[Feature Request]: Expose AbortController to be able abort request programmatically #844

Closed mskocik closed 10 months ago

mskocik commented 10 months ago

Describe the problem 🧐

There is no way to programatically abort currently running request. For example when you want to display dialog for long running request if you want to abort given request.

Only way to cancel request is by setting timeout, which cannot be used for use case described.

Describe the propsed solution 😎

Solution is simple, just add AbortController ~to options~ as a third parameter for fetch:request hook.

https://github.com/swup/swup/blob/d2b0d8f5a0ad33ea0127e070f921aa01c7f0a464/src/modules/fetchPage.ts#L72-L77

Alternatives considered 🤔

There is no alternative I can think of 🤷‍♂️

How important is this feature to you? 🧭

I cannot use swup without it

Checked all these? 📚

daun commented 10 months ago

Interesting use case! With the recent introduction of an official visit:abort hook, I'd prefer extending that to allow aborting the whole visit instead of just the request.

In any case, you should currently be able to replace the internal hook and supply your own fetch request with a custom AbortController you'll have access to.

swup.hooks.replace('fetch:request', (visit, { url, options }) => {
  const { signal } = new AbortController();
  return fetch(url, { ...options, signal });
});

While this will disable the internal timeout feature from working, you could look into combining multiple AbortControllers into a single signal or re-implement a timeout inside the replaced hook.

hirasso commented 10 months ago

I vote for making visit.abort() public.

mskocik commented 10 months ago

@daun Thanks for pointing that I can replace hook with custom implementation, I wasn't aware of this. I must have missed it in the docs (not sure it's there). But I believe it should be possible to do it natively, not by replacement, which would disable another built-in feature.

You can think of it as mimicking native browser functionality, that by pressign ESC you can stop current navigation. It's not possible to do out-of-the-box with swup. All 'abort' functionality is related to visit:abort hook, which is different use case - changing current navigation by another one, but not pure abort

@hirasso after looking into the code (just changing the state), not sure it is enough (but would be nice API if it handle abort correctly)

hirasso commented 10 months ago

@mskocik of course, you are right, that wouldn't be enough. I'm wondering - have you tried calling swup.navigate(swup.visit.from) ?

Never tried this myself but I'm curious what would happen.

Of course, if you really want to abort the actual request, I'd say there is no way around exposing the AbortController somehow.

daun commented 10 months ago

@mskocik What you're trying to do should already be possible. It's just that some of the methods aren't part of the public API yet. We'll mark visit.abort public in the next release, but it's already there. The navigation lifecycle is aware of the visit's state: aborting the visit before the page loaded will cancel the visit, aborting after the page has loaded will finish it normally. You can access the currently running visit on the swup instance.

const swup = new Swup();

// Cancel running visit when pressing escape
document.addEventListener('keydown', (event) => {
  if (event.key === 'Escape') {
    swup.visit?.abort();
  }
});

Hook replacement is intentionally left undocumented as it's an advanced feature and a bit of a potential foot gun. We could/should have marked it internal as it's in use mostly by official plugins and doesn't make a lot of sense anywhere else.

daun commented 10 months ago

@hirasso Can you think of any downsides of exposing the AbortController in the fetch:request hook?

mskocik commented 10 months ago

@daun

I'm wondering - have you tried calling swup.navigate(swup.visit.from) ?

It RE-fetches current page.

You can access the currently running visit on the swup instance.

Calling abort() doesn't undo navigation state (new url), don't reset progressbar, because visit:end is not called

Hook replacement is intentionally left undocumented as it's an advanced feature and a bit of a potential foot gun.

I agree it's potentially dangerous, but could be mentioned somewhere as an "advanced" feature.

What you're trying to do should already be possible.

I agree, thanks to ability to replace hook implementation, work-around exists.

mskocik commented 10 months ago

btw are you sure, abort works as it should? When changing navigation to different page the first request still finishes (status 200), but only second page is shown. Or is it by design? EDIT: Yes it is by design, because there is currently no way to abort running fetch

image

mskocik commented 10 months ago

Okay, after testing with providing my own AbortController as @daun suggested, I am getting uncaught error with this stack and swup breaks. Seems like async hook doesn't expect error to be thrown.

image

daun commented 10 months ago

are you sure, abort works as it should

No, we're not sure, but we appreciate you battle-testing it 🤠 Aborting visits is a recent feature meant to gracefully handle new visits when a visit is already running. Aborting a visit out of the blue the way you're suggesting is something that wasn't on our minds, so I'm pretty sure it's not 100% suited for that.

When changing navigation to different page the first request still finishes (status 200), but only second page is shown. Or is it by design?

That's definitely by design. Considering this from an animation and user experience standpoint, in most cases you're looking to ignore the running visit and execute the new visit if a new link was clicked in the meantime to achieve smooth animations.

Calling abort() doesn't undo navigation state (new url), don't reset progressbar, because visit:end is not called

Those are good points. Undoing the new URL is something to consider. The progress bar hiding on visit:abort is something that wasn't required with the use case we had in mind, but makes perfect sense. Feel free to create an issue or, even better, a PR, over on the progress plugin repo.

daun commented 10 months ago

Okay, after testing with providing my own AbortController as @daun suggested, I am getting uncaught error with this stack and swup breaks. Seems like async hook doesn't expect error to be thrown.

Can you share the exact code you're using to implement this? Hard so say for sure what's missing here.

daun commented 10 months ago

I've created a new issue with a summary of this discussion and a few suggestions on how and whether to tackle this.

Closing, continued in #847.

mskocik commented 10 months ago

Can you share the exact code you're using to implement this? Hard so say for sure what's missing here.

Sorry for delayed answer...

swup.hooks.replace('fetch:request', function(visit, { url, options }) {
  const abortController = new AbortController();
  setTimeout(() => {
    abortController.abort();
  }, 500)

  return fetch(url, { ...options, signal: abortController.signal });
});
daun commented 10 months ago

@mskocik This is a bit of a hack and could break in future releases, but for the time being you could try throwing { aborted: true } after aborting the fetch. That's how swup itself throws this and how it knows to ignore the exception. Let us know if that works.

setTimeout(() => {
  abortController.abort();
  throw { aborted: true };
}, 500)
mskocik commented 10 months ago

In console: image

Network fetch request is aborted correctly, but fetch:error is not being called.

daun commented 10 months ago

Then we're all out of options :) I'll try this out in my local playground to see how to enable your use case, but it probably needs to be implemented as a feature, if we want this functionality in the core.

mskocik commented 10 months ago

I think it would/could be enough if the error is being handled or propagated when async hook is being triggered.

daun commented 10 months ago

Propagating errors in hook handlers needs to be conditional on the type of exception; we need to tell the difference between error exceptions and exceptions thrown by aborting a request. One should crash and force a native browser reload, one should just be dropped silently. I'm not sure why it doesn't recognize the example code above as an aborted request; will need to look into that.

daun commented 8 months ago

@mskocik After giving this an earnest try, we've decided not to implement this for the time being — the added complexity is too high for a feature we'll need to maintain going forward. Thanks again for bringing it up and we'll let you know if the feature ever gets revived!

mskocik commented 8 months ago

@daun thanks for the effort. I understand, because when I was looking into it myself, I wasn't able to figure it out with constraints of current architecture.