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
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 26 additions & 3 deletions _internal/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,15 @@ import type { defaultConfig } from './utils/config'
export type GlobalState = [
Record<string, RevalidateCallback[]>, // EVENT_REVALIDATORS
Record<string, [number, number]>, // MUTATION: [ts, end_ts]
Record<string, [any, number]>, // FETCH: [data, ts]
Record<
string,
[
data: any,
timestamp: number,
revalidateCount: number,
abort: AbortController | undefined
]
>, // FETCH: [data, timestamp, revalidateCount, abort]
Record<string, FetcherResponse<any>>, // PRELOAD
ScopedMutator, // Mutator
(key: string, value: any, prev: any) => void, // Setter
Expand All @@ -14,15 +22,24 @@ export type FetcherResponse<Data = unknown> = Data | Promise<Data>
export type BareFetcher<Data = unknown> = (
...args: any[]
) => FetcherResponse<Data>
/**
* Second parameter provided to fetcher functions. At present this only allows
* for 'signal' for cancelling a request in progress, but allows for future
* enhancements.
*/
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.

}
export type Fetcher<
Data = unknown,
SWRKey extends Key = Key
> = SWRKey extends () => infer Arg | null | undefined | false
? (arg: Arg) => FetcherResponse<Data>
? (arg: Arg, options: FetcherOptions) => FetcherResponse<Data>
: SWRKey extends null | undefined | false
? never
: SWRKey extends infer Arg
? (arg: Arg) => FetcherResponse<Data>
? (arg: Arg, options: FetcherOptions) => FetcherResponse<Data>
: never

export type BlockingData<
Expand Down Expand Up @@ -200,6 +217,12 @@ export interface PublicConfiguration<
* @link https://swr.vercel.app/docs/advanced/react-native#customize-focus-and-reconnect-events
*/
isVisible: () => boolean

/**
* 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
Comment on lines +220 to +225

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.

}

export type FullConfiguration<
Expand Down
63 changes: 59 additions & 4 deletions core/use-swr.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { useCallback, useRef, useDebugValue, useMemo } from 'react'
import { useCallback, useRef, useDebugValue, useMemo, useEffect } from 'react'
import { useSyncExternalStore } from 'use-sync-external-store/shim/index.js'

import {
Expand All @@ -25,6 +25,7 @@ import {
import type {
State,
Fetcher,
FetcherOptions,
Key,
SWRResponse,
RevalidatorOptions,
Expand Down Expand Up @@ -104,6 +105,10 @@ export const useSWRHandler = <Data = any, Error = any>(

const stateDependencies = useRef<StateDependencies>({}).current

// When starting a revalidate, record an abort callback here so that it
// can be called if a newer revalidation happens.
const abandonRef = useRef<(() => void) | undefined>()

const fallback = isUndefined(fallbackData)
? config.fallback[key]
: fallbackData
Expand Down Expand Up @@ -271,6 +276,7 @@ export const useSWRHandler = <Data = any, Error = any>(
unmountedRef.current ||
getConfig().isPaused()
) {
abandonRef.current?.()
return false
}

Expand Down Expand Up @@ -341,17 +347,58 @@ export const useSWRHandler = <Data = any, Error = any>(
}, config.loadingTimeout)
}

const options: FetcherOptions = {}
let abortController: AbortController | undefined

if (
config.abortDiscardedRequests &&
typeof AbortController !== 'undefined'
) {
abortController = new AbortController()
options.signal = abortController.signal
}

// Start the request and save the timestamp.
// Key must be truthy if entering here.
FETCH[key] = [
currentFetcher(fnArg as DefinitelyTruthy<Key>),
getTimestamp()
currentFetcher(fnArg as DefinitelyTruthy<Key>, options),
getTimestamp(),
0, // Number of revalidate calls waiting on this fetcher result
abortController
]
}

const currentRequestInfo = FETCH[key]

// Reference count the number of revalidate calls that are awaiting this request
currentRequestInfo[2] += 1
abandonRef.current?.()

if (config.abortDiscardedRequests) {
// Store an abort callback in abortRef to be called if/when the next revalidate happens
// or if the component is unmounted. Will only do anything the first time its invoked.
let abandoned = false

abandonRef.current = () => {
if (!abandoned) {
abandoned = true
// Decrement the reference count
currentRequestInfo[2] -= 1
// If this was the last awaiter then abort the request
if (currentRequestInfo[2] == 0) {
// Note: we may consider it OK to abort a request thats already
// resolved as its basically a no-op. The other option is to clear
// the abortcontroller immediately after the await so that its not
// aborted.
currentRequestInfo[3]?.abort()
}
}
}
}

// Wait until the ongoing request is done. Deduplication is also
// considered here.
;[newData, startAt] = FETCH[key]
;[newData, startAt] = currentRequestInfo
newData = await newData

if (shouldStartNewRequest) {
Expand Down Expand Up @@ -627,6 +674,14 @@ export const useSWRHandler = <Data = any, Error = any>(
}
}, [refreshInterval, refreshWhenHidden, refreshWhenOffline, key])

// On final unmount
useEffect(
() => () => {
abandonRef.current?.()
},
[]
)

// Display debug info in React DevTools.
useDebugValue(returnedData)

Expand Down
3 changes: 2 additions & 1 deletion test/use-swr-middlewares.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { act, screen } from '@testing-library/react'
import React, { useState, useEffect, useRef } from 'react'
import type { Middleware } from 'swr'
import useSWR, { SWRConfig } from 'swr'
import type { FetcherSecondParameter } from 'swr/_internal'
import { withMiddleware } from 'swr/_internal'

import {
Expand Down Expand Up @@ -242,7 +243,7 @@ describe('useSWR - middleware', () => {
function Page() {
const { data } = useSWR(
[key, { hello: 'world' }],
(_, o) => {
(_, o: FetcherSecondParameter & { hello: string }) => {
return o.hello
},
{
Expand Down