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 use Fetch API adapters during client creation. #1973

Closed
ProdanSergey opened this issue Jul 26, 2023 · 11 comments
Closed

Comments

@ProdanSergey
Copy link

ProdanSergey commented Jul 26, 2023

Expected Behavior

Abstract the current implementation of the Axios adapter and allow the creation of a contentful client with a given adapter of HTTP Client.

Actual Behavior

Currently, it's not possible to instantiate a client using a Fetch as an adaptor for HTTP Client to communicate with Contentful Rest API. It's quite clear that contributors do not have a strong attitude to moving away from Axios, however, it's really crucial to be able to use other implementations of HTTP Clients. For instance: look at this known issue with SvelteKit SSR implementation. Their approach to handling the hydration step heavily depends on the custom implementation of a Fetch API. As a consequence of not being able to use Fetch as a Contentful client adapter developers struggling with repetitive API calls. There are no ways to solve this issue rather than use the exact implementation of Fetch.

Hope that does make sense and you can see community demand.

Thank you.

@marcolink
Copy link
Member

Hi @ProdanSergey,

have you already tried to set an adapter upon client creation?https://gist.github.com/marcolink/2c29479999bc1214e5f11f61baf5b38a?

@ProdanSergey
Copy link
Author

ProdanSergey commented Jul 26, 2023

Hello, @marcolink. Thanks for your response. I tried the proposed one, unfortunately, it does not support Node. it's written in the readme.

Since this adapter relies on fetch API so it won't work in Node environment.

Do you mind providing any instructions on how to adapt:

interface AxiosAdapter { (config: AxiosRequestConfig): AxiosPromise; }

into

interface FetchAdapter { (config: Request): Response; }

I'll appreciate it a lot. However with provided typing i don't think it's possible

@marcolink
Copy link
Member

The type for the adapter field is coming directly from axios here.
contentful.js is still on [email protected] (this hopefully changes soon 🤞), what version of axios did you define in your package.json?

@ProdanSergey
Copy link
Author

Hope you folks considered making ContenfulApiClient['adapter'] more generic than just an alias of AxiosAdapter.

Anyway, thanks for the efforts. I'm gonna try to map node-fetch to Axios manually.

@marcolink
Copy link
Member

Not sure if I understand what you mean by

making ContenfulApiClient['adapter'] more generic than just an alias of AxiosAdapter

@ProdanSergey
Copy link
Author

ProdanSergey commented Jul 26, 2023

Sry my fault I put the wrong type:

So what I think would be more useful is to provide a more generic type for the request function (which I suppose stands for AxiosAdapter)

My proposal would be:

type CreateClientParams<Request, Response> = { ... adapter?: Adapter<Request, Response> }

Then theoretically it's easy to adapt Axios on your side as

class AxiosAdapter implements Adapter<AxiosRequestConfig, AxiosResponse> {
   ...
   
   request():  Promise<AxiosResponse> {}
}

Meanwhile, library consumers would have more chance to adapt to other HttpClients without the hassle of mapping requests and responses.

@marcolink
Copy link
Member

The adapter interface is the official interface provided by axios.
This allows for assigning all existing 3rd party adapters.
They also provided some docs on how to write your own - maybe this is worth checking out!

In general, axios works wunderbar on node environments. There might be limitations on edge runtimes though.

@ProdanSergey
Copy link
Author

But I don't understand why Contentful SDK dictating consumers to adopt its dependency (in this case Axios, no matter how good it works).

The adapter configuration should accept a RequestHandler function. In my opinion, the library should abstract all its dependencies and give a very generic interface to set consumer configuration.

@marcolink
Copy link
Member

Thanks for your feedback @ProdanSergey 🙏
Im happy to forward your request to out product team.

I will close this isssue for now, please feel free re-open it if you have more related questions.

@robsterlini
Copy link

We ran into a similar issue trying to use the Contentful SDK in a server rendered component on the Edge and this was our workaround using the new [email protected] which includes fetch as a valid option for getAdapter (added in axios/axios#6371):

/**
 * Edge doesn't support axios' default adapter (http). It also requires an extra
 * workaround to handle the broken UA string that the Contentful SDK sends.
 * Presumably the SDK is expecting a version number in the UA string, so we
 * just add a version number to make it work – in our case Node 20 as it matches
 * the version of Node we are using.
 */
const edgeAdapter: AxiosAdapter = (config) => {
  const validUA = config.headers['X-Contentful-User-Agent'].replace(
      'node.js/undefined',
      'node.js/20'
  );

  const headers = structuredClone(config.headers);
  headers['X-Contentful-User-Agent'] = validUA;

  return axios.getAdapter('fetch')({ ...config, headers });
};

const client = createClient({
    // Other values…
    adapter: isEdgeRendering ? edgeAdapter : undefined
});

@robsterlini
Copy link

We ran into a similar issue trying to use the Contentful SDK in a server rendered component on the Edge and this was our workaround using the new [email protected] which includes fetch as a valid option for getAdapter (added in axios/axios#6371):

/**
 * Edge doesn't support axios' default adapter (http). It also requires an extra
 * workaround to handle the broken UA string that the Contentful SDK sends.
 * Presumably the SDK is expecting a version number in the UA string, so we
 * just add a version number to make it work – in our case Node 20 as it matches
 * the version of Node we are using.
 */
const edgeAdapter: AxiosAdapter = (config) => {
  const validUA = config.headers['X-Contentful-User-Agent'].replace(
      'node.js/undefined',
      'node.js/20'
  );

  const headers = structuredClone(config.headers);
  headers['X-Contentful-User-Agent'] = validUA;

  return axios.getAdapter('fetch')({ ...config, headers });
};

const client = createClient({
    // Other values…
    adapter: isEdgeRendering ? edgeAdapter : undefined
});

Update: this worked locally with a pseudo-edge, but broke when it got to Vercel. We've abandoned this for now and are gunna look at different options. Axios as a baked in dependency is quite problematic when using it on the Edge.

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

No branches or pull requests

3 participants