Signal from fetchOptions not passed to fetch

[rexpan]

Describe the bug

import { createClient, cacheExchange, fetchExchange } from "@urql/core"
export const client = createClient({
    url: "https://api/graphql",
    exchanges: [cacheExchange, fetchExchange],
    requestPolicy: "cache-and-network",
    fetch,
})
const { signal } = new AbortController
function fetch(input, init) {
    console.assert(init.signal === signal)
}
await client.query(gql`...`, {}, { fetchOptions: {signal} })

Expect the signal from fetchOptions should be passed to fetch and user can control/abort the query. Actually the signal has been override from fetchSource.ts

export function makeFetchSource(
  operation: Operation,
  url: string,
  fetchOptions: RequestInit
): Source<OperationResult> {
  let abortController: AbortController | void;
  if (typeof AbortController !== 'undefined') {
    fetchOptions.signal = (abortController = new AbortController()).signal;
  }
  return pipe(
    fromAsyncIterable(fetchOperation(operation, url, fetchOptions)),
    filter((result): result is OperationResult => !!result),
    onEnd(() => {
      if (abortController) abortController.abort();
    })
  );
}

Suggest to also abort if the signal passed from user aborted.

export function makeFetchSource(
  operation: Operation,
  url: string,
  fetchOptions: RequestInit
): Source<OperationResult> {
  let abortController: AbortController | void;
  if (typeof AbortController !== 'undefined') {
+    if (fetchOptions.signal) fetchOptions.signal.addEventListener('abort', () => abortController?.abort());
    fetchOptions.signal = (abortController = new AbortController()).signal;
  }
  return pipe(
    fromAsyncIterable(fetchOperation(operation, url, fetchOptions)),
    filter((result): result is OperationResult => !!result),
    onEnd(() => {
      if (abortController) abortController.abort();
    })
  );
}

Reproduction

https://stackblitz.com/edit/github-b9nibt?file=src%2Fclient.js

Urql version

urql v4.1.2

Validations

• [X] I can confirm that this is a bug report, and not a feature request, RFC, question, or discussion, for which GitHub Discussions should be used
• [X] Read the docs.
• [X] Follow our Code of Conduct

[kitten]

I don't really see this as idiomatic. Essentially, I'm assuming you're referring to your question from Discord: https://discord.com/channels/1082378892523864074/1150947449330999296/1150947449330999296 Just on a side-note, please use some patience, because now we've got three places to talk about this already instead of one for an initial chat 😅 Anyway, I'll leave a link there to this.

First off, I'm pretty opposed to the changes as proposed in #3378 and re-using fetchOptions.signal. There's multiple reasons for this.

• From a perspective of @urql/core, fetch/HTTP is not an assumption. In a perfect world fetchOptions wouldn't even exist, but it's necessary in some cases and for ease of use. So, fetchOptions.signal — and similar mechanisms — should not have an influence on control flow
• While it seems to make sense to pass on an AbortSignal, where would that signal reasonably come from?
  • If it comes from the UI, it's a UI concern and the mechanism to abort already exists and shouldn't be modelled as an AbortSignal, since all operations are modelled as cancellable sources
  • If it comes from another exchange, there's already a mechanism for this in the form of active teardowns
  • If this is actually for timeouts, then a result must be issued, but given that a signal is added, it makes more sense to issue a timeout result while issuing an active teardown forwards

So, all in all, I've seen people solve this by passing in a custom fetch function in the past — which to an extent is reasonable, because we're customising the behaviour of just the fetch procedure. I'm not opposed in general to discuss some tiemout functionality via an exchange or other mechanisms, but, in general, just accepting an AbortSignal via fetchOptions I see as, while logical, superfluous, since there's no clear usage pattern that'd create this AbortSignal in reasonable user code, unless we find a usage pattern that would warrant this.

[tatianajiselle]

Hey there @kitten thanks for your responses as ive seen youre quite knowledgeable about exchanges/core etc..

I think your link to discord is actually to my question (: -- i updated my question to link here as well.

Tho it appears this might be the reason to why abort controller hasnt worked for setting a timeout.

EDIT:

can I have some guidance for implementing a fetch wrapper? ive tried pulling in the fetch function from undici and passing it in to client.fetch but it still requires the abort signal to set a timeout and its still not getting fired.

im not sure if im missing something 🤔 (possibly some conceptual knowledge) but what would a wrapper implementation look like?

import { fetch } from "undici";
...

       function fetchWithTimeout(url: any, opts: any): Promise<any> {
            const controller = new AbortController();
            setTimeout(() => controller.abort(), 50);

            return fetch(url, {
                ...opts,
                signal: controller.signal,
            });
        }

      this.client = new Client({
          url: this.url
          exchanges: [retryExchange(retryOptions), fetchExchange],
          fetch: fetchWithTimeout,
          requestPolicy: "network-only",
      });

[rexpan]

@tatianajiselle My workaround would be define a fetchOptions with a different signal

const signal: AbortSignal;
client.query(query, variables, { fetchOptions: { signal, _signal:signal })

And client is passed with custom _fetch

const client = createClient({ fetch: _fetch })
function _fetch(input: RequestInfo | URL, init?: RequestInit) {
    if (init?._signal) {
        if (!init.signal) init.signal = init._signal
        else if (init._signal !== init.signal) init.signal = combineSignals([init.signal, init._signal])
    }
    return fetch(input, init)
}

function combineSignals(signals: AbortSignal[]) {
    const ctrl = new AbortController()
    for (const signal of signals) {
        signal.addEventListener("abort", () => ctrl.abort(), { once: true })
    }
    return ctrl.signal
}

[rexpan]

there's already a mechanism for this in the form of active teardowns

Could you correct me how to use it? Cannot found in the doc. Below is my assumption

const operation = client.createRequestOperation('query', createRequest(query, variables), { fetchOptions: { headers:{} } })
const p = client.executeRequestOperation(operation).toPromise()

// when you need to abort the current request
client.executeRequestOperation(makeOperation("teardown", operation))

[kitten]

@rexpan In principle, when actively cancelling an operation you can call client.reexecuteOperation with your teardown operation.

That said, that's to actively interrupt an operation that's ongoing, so that’s not to cancel what the exchanges are doing, but that will actively interrupt an operation entirely and interrupt all bindings from receiving updates as well.

What I'd specifically recommend if you simply want a timeout on fetch is to write a wrapper function around fetch and combine the signal that fetch receives with your own timeout signal.

If you instead want to interrupt on a timeout with an exchange, the exchange would have to send a result while forwarding a teardown operation. However, again, the fetch wrapper is simpler in this case and keeps the timeout as a fetch concern.

const fetchTimeout = async (url, options) => {
  const controller = new AbortController();

  options.signal.addEventListener('abort', () => {
    controller.abort();
  });

  setTimeout(() => {
    controller.abort();
  }, TIMEOUT);

  return fetch(url, { ...options, signal: controller.signal });
};

I haven't tested the above, but something along the lines of this would be the simplest way to add a timeout to fetch

[tatianajiselle]

@kitten this worked for me -- thank you for the insight, support and suggestion!!