apollo-link-rest

apollo-link-rest copied to clipboard

Hello all 👋

I'm currently trying out apollo-link-rest via a node script, and ran into issues with how it expects to use Headers class off of the global object. I'm currently using the cross-fetch package to pull in a fetch ponyfill and passing it into the RestLink constructor via the customFetch option. However there doesn't seem to be a way to do that with the Headers object as well. I'm currently just fully polyfilling the Headers class on the global object, but would like to be able to pass it in via the constructor on a customHeaders option instead.

Proposal:

const { fetch, Headers} = require("cross-fetch");
const { RestLink } = require("apollo-link-rest");

const restLink = new RestLink({
	... // other options
	customFetch: fetch,
	customHeaders: Headers,
});

Workaround:

const { fetch, Headers} = require("cross-fetch");
const { RestLink } = require("apollo-link-rest");

global.Headers = Headers;

const restLink = new RestLink({
	... // other options
	customFetch: fetch,
});

For the sake of easily replicating this, I've added a repository that reproduces this: https://github.com/basicdays/ghtest

Generally speaking, the Headers API is supposed to be fully supported -- except on IE -- and IE needs lots of hacks anyhow, so patching in a Polyfill there should be the norm. (Note: iOS Safari does support it, no idea why can-i-use is wrong there)

Would you mind elaborating more why you want this feature @basicdays?

-- The customFetch API allows for replacing the fetch implementation, but really it was added to ApolloLinkRest as an escape hatch, so you could inject testing, or middleware that you needed at the network layer -- that doesn't seem super necessary in the case of Headers.

I don't expect that this would be a crazy difficult feature to add, but I'd love to have a specific use-case before adding more configuration hooks like this.

That's definitely true if running the code only in browsers. However if apollo-link-rest code is ran in Node.js, then a lot of the browser APIs are not available, such as fetch and Headers. Adding the ability to at least pass in a Headers ponyfill without having to polyfill the global Node.js seems to be all that would be needed to make this package work in environments beyond just browsers.

It looks like Apollo Client v3 kind of expected that kind of environment with this code: https://github.com/apollographql/apollo-client/blob/main/src/link/http/checkFetcher.ts#L3

@basicdays I had definitely not considered using apollo-link-rest much from the server-side-rendering side of things. -- If you're doing SSR I guess I imagined you could stand up an ApolloServer that reads from a REST source.

I wouldn't heavily object to a configuration option like this for that purpose, but I don't recommend we make it pluggable for every endpoint -- if you want to implement this, I'd accept a PR that allows configuration at the Link-Level (parallel to customFetch)

@basicdays I had definitely not considered using apollo-link-rest much from the server-side-rendering side of things. -- If you're doing SSR I guess I imagined you could stand up an ApolloServer that reads from a REST source.

I wouldn't heavily object to a configuration option like this for that purpose, but I don't recommend we make it pluggable for every endpoint -- if you want to implement this, I'd accept a PR that allows configuration at the Link-Level (parallel to customFetch)

In this case, I'm using it in a CLI tool to work with the Github Actions CI, since half of their API is in GraphQL, and half of it is still in Rest. I'd agree that doing it at the @rest directive level is probably way out of scope. I'll see if I can get a PR sometime this week. Just to double check, I'm assuming adding tests are not required for a PR on this repo?

Side note, I apologize about the influx of issues. I figured it was better to at least report them than just workaround them on my own. :)

When possible, tests are appreciated -- we do have tests for many common situations. That said, we don't have integration tests, so something like this Headers thing may not directly need tests -- Let's see how trivial the PR is!

No problem re: filing tickets, reporting them is the only way we can keep track of what people care about or need support for!