RFC: Async Client fetchOptions support

[StevenLangbroek]

Summary

We use Keycloak as a service-to-service authentication layer, and want to use URQL on server. Grabbing a (JWT) access token means reaching out to it over the network, and tokens have a low TTL with a refresh token. The way we could resolve this with current implementation is "hacky", in pseudo-code:

let token;

const client = createClient({
  url: 'http://bla.bla',
  fetchOptions: () => ({
    headers: {
      Authorization: `Bearer ${token}`
    }
  }),
});

// here's where it gets awks

getToken().then(t => {
  const jwt = decode(t)
  token = jwt.access_token
  setTimeout(() => {
	t = null;
    getToken();
  }, jwt.expiration_date).unref() // just to sketch, it's not how it works obvsly.
});

This kind of works (I know it can be made to work correctly), but there's a real chance that if this server is under high load that some requests miss their token while a new one is being requested. Making this code work just gets more and more awkward...

Proposed Solution

I understand this has potential performance implications, but what if fetchOptions's function form has this signature instead of the existing one?

fetchOptions: RequestInit | () => RequestInit | () => Promise<RequestInit>

This would quite elegantly solve our problem:

const client = createClient({
  url: 'http://bla.bla',
  fetchOptions: async () => ({
    headers: {
      Authorization: `Bearer ${await getOrFetchToken()}`
    }
  })
})

Requirements

• Should not break existing behavior
• Existing behavior should take as little of a performance hit as possible.

[JoviDeCroock]

Hey,

I like the idea since it solves cases for react-native,... as well, if only the fetchExchange has to worry about this we should be fine.

Just a sidenote: at this moment in time we advise this to be solved in an exchange https://formidable.com/open-source/urql/docs/common-questions/#how-do-we-achieve-asynchronous-fetchoptions

[StevenLangbroek]

Huh... that's pretty elegant... I had missed that in the docs. This'll need a @ts-ignore comment though I think, right?

[JoviDeCroock]

Yes, I guess so. Was thinking if we could change that but it would be bad for us to assume everyone knows they have to add that then.

[StevenLangbroek]

If you want I can take a stab at implementing the change over the weekend if you think it has merit. Let me know :)

[kitten]

Just linking some related issues:

• How can you create an exchange that delays the pipeline until a promise is resolved #327 (Advise to create a "map context exchange")
• It would be really useful if fetchOptions could allow to return a promise #326 (Same Guidance)
• Feature request: allow to return promise in fetchOptions function #234 (Same Request)

I think to explain this, this was hard for us to make a decision on, because philosophically two of our ideals are at odds with that request:

1. Extending fetchOptions makes sense and helps simplicity, i.e. this API is already there and we're only limiting this to encourage a more solid exchange to be written
2. Adding this is at odds with our hackability / composability ideals, which dictate that all APIs should be minimal in the Client

The problem here fundamentally is that fetchOptions is defined on the context as a function, but is actually evaluated on all fetchExchanges. This means that it's a piece of configuration and not a piece of "control flow logic" to us.

I think there are good arguments to finally add this though, since we now have a central place to put it, but the concerns around guiding people to use more exchanges still stands. But as it currently is, we don't provide something like a @urql/exchange-utils package with exchanges like a mapContextExchange, a mapResultExchange, or filterResultExchange, i.e. utilities that would encourage these small pieces of logic to be extracted.

[StevenLangbroek]

Hmmm... Question; for NodeJS usage, would you generally recommend creating a client instance per request (or event etc)?

[kitten]

@StevenLangbroek Yes, otherwise it’s pretty likely to accidentally leak authenticated data (unless your SSR instance never sends a token of course)

Edit: Actually, in good old Redux-land, I used to have either cars frequently; where SSR servers hurt render unauthenticated app-shells sometimes

[kitten]

I think we now have two competing ideas in here, but with the addition of #815 it may actually alleviate the underlying problem.

We also have #796 but aren't sure yet whether that'll go forward and be finalised.

The reason why fetchOptions isn't that extensive yet is that it's a configuration option in operation.context. Technically all of these configurations, apart when fetchOptions is a function, are stateless, because exchanges can interpret these options. So we're wary of encouraging this exception and widening it by adding async fetchOptions

[StevenLangbroek]

I understand. I'm totally fine with abstracting client creation away into an async function that happens on every request, rather than overloading fetchOptions further. Feel free to close. Thanks for the thoughtful discussion 🙏

[kitten]

@StevenLangbroek that sounds like it’d clear your cache on every request as well 😅 Have you seen this Gist? It’ll probably soon be integrated into the docs as a “recipe” to write a more complex auth exchange. https://gist.github.com/kitten/6050e4f447cb29724546dd2e0e68b470

[StevenLangbroek]

Oh interesting, I'll share that with colleagues. Thanks! For us requests are all user-bound anyway so sharing cache between requests would potentially have security implications.

[kitten]

This will hopefully be solved by the more specific approach of #939

[kitten]

Closed since #939 is merged. Let's see how that goes for now! 🙌