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

Feature request: allow to return promise in fetchOptions function #234

Closed
capaj opened this issue Apr 30, 2019 · 17 comments
Closed

Feature request: allow to return promise in fetchOptions function #234

capaj opened this issue Apr 30, 2019 · 17 comments

Comments

@capaj
Copy link

capaj commented Apr 30, 2019

there are often cases when in order to obtain a JWT token, we have to call an async function. For example with AWS amplify: https://aws-amplify.github.io/docs/js/authentication#retrieve-current-session

it would be nice if urql would allow us to return fetch options from the fetch asynchronously.

currently fetch options type is defined here: https://github.com/FormidableLabs/urql/blob/9a49818c4a54583d1f457772323dab58c78211df/src/client.ts#L32

and as you can see it only accepts a static object or a function which returns synchronously.

@capaj capaj changed the title allow to return promise in fetchOptions function Feature request: allow to return promise in fetchOptions function Apr 30, 2019
@kitten
Copy link
Member

kitten commented Apr 30, 2019

So, it makes complete sense to delay the queries a little depending on your environment, but fetchOptions is mostly a convenience helper that's leftover from previous versions.

It might make more sense for you to write an exchange to manipulate the queries' context.

We haven't provided a helper for this yet, but in the meantime let me know if you'd like any code snippet for achieving this?

@capaj
Copy link
Author

capaj commented Apr 30, 2019

yes please. AlthoughI have to ask-do you really believe that forcing users to write their own exchange even for such a basic usecase is desirable? Users migrating from apollo-client will have hard time grokking the whole wonka API.

@kitten
Copy link
Member

kitten commented Apr 30, 2019

@capaj No, we wouldn't force anyone to write exchanges at any time. On the other hand it stands to reason that urql is about customisability and a minimal core.

My theory is that once we provide a helper for errors and changing the context, then it'll become unnecessary for most though to write their own exchanges.

This would then be similar to Apollo's apollo-link-error and apollo-link-context

@andyrichardson
Copy link
Contributor

andyrichardson commented May 31, 2019

@capaj Depending on how your Auth/routes are setup, there's a good chance you could:

  • store the client in state and update the state to a new client once the Auth token is available
  • use two separate contexts (and clients) for Auth and non-auth routes

I'm not a huge fan of having async fetchOptions but I can see value in being able to update the client's fetch options. This shouldn't be too hard to implement in the client and also the provider.

@andyrichardson
Copy link
Contributor

Looks like this could be a valid use case. I'll have a play around with it this weekend.

@andyrichardson
Copy link
Contributor

@capaj I've created a PR (#264) but while trying it out, it got me wondering whether a setter/async fetchOptions are required for your use case.

We already support functional implementations of fetchOptions which are called at every fetch call. You could provide a function which gets the access token from localStorage (see below example).

// Client declaration
const client = createClient({
  //..
  fetchOptions: () => { headers: { 'Some-Auth-Header': window.localStorage.getItem('some-auth-token') } }
});

Looking at the docs for your Auth provider, you shouldn't need to manually request a session key / refresh (source).

When using Authentication with AWS Amplify, you don’t need to refresh Amazon Cognito tokens manually. The tokens are automatically refreshed by the library when necessary.

Security Tokens like IdToken or AccessToken are stored in localStorage for the browser

Let me know if the above example works for you.

@RossWilliams
Copy link

@andyrichardson The docs are correct that there is no manual refresh, but its the auto-refresh functionality supplied by Amplify which forces async access patterns to the current JWT. This is the normal access pattern for Amplify:

const token = (await Auth.currentSession())
          .getAccessToken()
          .getJwtToken();

the async call to currentSession() will refresh tokens if necessary on access. I'm not aware of a synchronous access pattern to Amplify's auth store without reading their namespace from localStorage, which is not a good pattern. In fact it works hard to prevent access to stale credentials.

The most straightforward solution I see is to modify fetch.ts. Call Promise.resolve(extraOptions) after extraOptions is defined, and have the rest of the method inside that promise's then function.

@kitten
Copy link
Member

kitten commented Jun 6, 2019

We were discussing a lot how best to make this work and we were actually thinking of building in an escape hatch. But another solution that we're currently favouring might be to add an exchange for this similar to this: https://www.apollographql.com/docs/link/links/context/

We might still expose a second escape hatch but the reason for this is that it might be pretty dangerous to attach more stateless functions to urql that aren't exchanges, especially if they're asynchronous. It does increase the surface area of things that might go wrong.

That's actually why this has taken a while but we'll likely ship a solution for this in the upcoming v1.1, hopefully this week.

I'm not quite sure whether this exchange pattern would work for you but it's essentially more powerful than extending the reach of fetchOptions 👍

@kitten
Copy link
Member

kitten commented Aug 1, 2019

Since this is a very small exchange we'll likely show just a guide and example of how to set this up as part of #247. If anyone else then publishes a "generic exchange" for this that'd be awesome!

@kitten kitten closed this as completed Aug 1, 2019
@papigers
Copy link

papigers commented Jan 1, 2020

yes please. AlthoughI have to ask-do you really believe that forcing users to write their own exchange even for such a basic usecase is desirable? Users migrating from apollo-client will have hard time grokking the whole wonka API.

+1 on this.
I don't understand why a new user to urql (such as myself) who requires to do something as simple as adding an authorization token asynchronously to the urql client should:

  1. Learn the whole wonka API.
  2. Learn how to write an exchange.

Even though wonka and writing exchanges are well-documented, they are still need to be learned, this is not an ideal solution for simple requirements such as this.
IMO exchanges should solve more complex scenarios - this is not one of them.

@kitten
Copy link
Member

kitten commented Jan 1, 2020

@papigers there are also alternative solutions, like adding tokens to a component that passes them on to a client that is kept in state.

We’re currently exploring how we can structure the project to ship more “utility exchanges”. Part of why we haven’t done that before is to make urql‘s bundlesize appear as small as it’ll realistically be. We’re now considering to ship more exchanges and communicate that treeshaking will enable the same size as before.

One concern has been that we do believe in browser ESM imports which would see larger files being downloaded as part of that.

Either way, we’ve got the following exchanges planned:

  • a “buffering” auth exchange with refresh support
  • a schema exchange that can be used to create a “faux GraphQL API” entirely in the app
  • a retry exchange to rerun queries and mutations on network errors with exponential back off

That being said, all this code exists alteady and you’re welcome to — for now — copy it yourself, open issues with questions and/or for help (or ask on Spectrum), and use it without learning any of Wonka.

IMO exchanges should solve more complex scenarios - this is not one of them.

Generally I totally agree and we need to alleviate this problem. On the other hand it’s worth mentioning that Exchanges will remain our main point of extensibility and implementation of features, so we encourage people to maintain third-party exchange packages while we work this out 🙌

Hope this helps!

@papigers
Copy link

papigers commented Jan 1, 2020

@kitten thanks for the quick reply.

I understand your desire to keep exchanges as the main point of extensibility, but I still think you need to differentiate between minor change/extension such as this which requirer minor tweaks in the core API, and bigger extensions that would be solved using exchanges, but we can agree to disagree :)

That being said, great package - well done :)

@RodolfoSilva
Copy link

RodolfoSilva commented Mar 23, 2020

I'm using this custom exchange and working fine:
Codesandbox: https://codesandbox.io/s/green-grass-u3ens

import {
  Provider,
  createClient,
  Exchange,
  Operation,
  dedupExchange,
  cacheExchange,
  fetchExchange,
  debugExchange
} from "urql";

import { pipe, mergeMap, fromPromise, fromValue, map } from "wonka";

const isPromise = (value: any) => value && typeof value.then === "function";

export const fetchOptionsExchange = (fn: any): Exchange => ({
  forward
}) => ops$ => {
  return pipe(
    ops$,
    mergeMap((operation: Operation) => {
      const result = fn(operation.context.fetchOptions);
      return pipe(
        isPromise(result) ? fromPromise(result) : fromValue(result),
        map((fetchOptions: RequestInit | (() => RequestInit)) => ({
          ...operation,
          context: { ...operation.context, fetchOptions }
        }))
      );
    }),
    forward
  );
};

const client = createClient({
  url: "https://countries.trevorblades.com/",
  exchanges: [
    dedupExchange,
    debugExchange,
    cacheExchange,
    // Must be called before fetchExchange and after all others sync exchanges, respecting the rule Synchronous first, Asynchronous last
    fetchOptionsExchange(async (fetchOptions: any) => {
      return Promise.resolve({
        ...fetchOptions,
        headers: {
          Authorization: "Bearer mySuperToken"
        }
      });
    }),
    fetchExchange
  ]
});

@kitten maybe somthing like this could be part of the core library or like the retryExchange.

@acro5piano
Copy link

Hi, I've created a tiny library that add headers asynchronously. Hope it helps someone.

https://github.com/acro5piano/urql-exchange-async-headers

@ritchieanesco
Copy link

Thanks @acro5piano for writing the helper.

@charlie0077
Copy link

Is it supported now? Switching to apollo-client because of this.

@acro5piano
Copy link

acro5piano commented Oct 8, 2023

Starting (maybe) URQL v4, async header can be implemented using mapExchange :

import { createClient, mapExchange, Operation, makeOperation } from 'urql'

const client = createClient({
  url: "https://countries.trevorblades.com/",
  exchanges: [
    cacheExchange,
    // Must be called before fetchExchange and after all others sync exchanges, respecting the rule Synchronous first, Asynchronous last
    mapExchange({
      async onOperation(operation) {
        return makeOperation(operation.kind, operation, {
          ...operation.context,
          fetchOptions: {
            ...operation.context.fetchOptions,
            headers: {
              Authorization: await Promise.resolve("Bearer mySuperToken")
            },
          },
        })
      },
    }),
    fetchExchange
  ]
});

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 a pull request may close this issue.

9 participants