Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 1933 - Cancellation of requests when no longer needed #2275

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

CaptainPants
Copy link

@CaptainPants CaptainPants commented Dec 12, 2022

First attempt at #1933 - adding a cancellation mechanism to fetchers.

Looking for feedback on the approach and if there is any huge landmines I'm missing. I won't be offended if this is just a big 'no'.

Still working on how to appropriately test this.

TODO:

  • Not sure how error handling works as this will cause the await to throw a DOMException from the abort call, and don't want this to send the hook into an error state.
  • Testing.
  • Tests - fix existing ones and add new ones related to the change.
  • Add an example?

@codesandbox-ci
Copy link

codesandbox-ci bot commented Dec 12, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit aa869c7:

Sandbox Source
SWR-Basic Configuration
SWR-States Configuration
SWR-Infinite Configuration
SWR-SSR Configuration

@CaptainPants CaptainPants changed the title Issue 1933 Issue 1933 - Cancellation of requests when no longer needed Dec 12, 2022
Comment on lines +209 to +225

/**
* Enable this to pass an instance of AbortSignal to your fetcher that will be aborted when there no useSWR hooks remaining that are listening for the result of the fetch (as
* a result for example of navigation, or change of search input). This allows you to cancel a long running a request if the result is no longer relevant, e.g. when doing a search.
*/
abortDiscardedRequests?: boolean

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like for this feature to be enabled by default. Not the other way around.

Let's emphasize what the new users want rather than reverse compatibility since the risk of problems is low.

Would a new user who has no prior version of swr installed prefer this on by default or not? I think yes but I'm only one user.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lest setting this to true give someone a false impression, I think this config should be removed altogether.

  1. Setting abortDiscardedRequests to true is insufficient for aborting discarded requests. Both of these also must be true
    a. The request API supports cancellation with AbortSignal (not all do)
    b. You remembered to pass the signal to that API
  2. Simply ‘not passing the signal to the request API’ is sufficient to disable the cancellation of discarded requests.

I think we should not offer this config, but rather always pass signal to the fetcher function.

If you're worried about constructing a bunch of AbortControllers that nobody will ever use (don't) you could always create a Proxy and only construct the AbortController when someone gets the signal property.

Removing personal email.
@davidnormo
Copy link

Is there anything missing from this preventing it from being merged?

@oanaOM
Copy link

oanaOM commented Feb 4, 2024

This would be very helpful to have! Any ideas when will this get released?
Meanwhile, is there other workaround to make SWR work with AbortController or achieve a cancelling mechanism?

@scarqin
Copy link

scarqin commented Apr 16, 2024

Exciting features! looking forward to launch.

*/
export interface FetcherOptions {
signal?: AbortSignal
[key: string]: unknown
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd omit an index signature because it may hide typos for the valid options (only signal in this case). It also prevents us from adding properties without introducing a potential breaking change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants