feat: allow per-request caching in RESTDataSource

[bennypowers]

implements protected shouldCacheRequest

fixes apollographql/datasource-rest#73

• ⏰ As implementing this feature was relatively quick, I skipped straight to the code, instead of waiting on consensus.
• 💡 Naturally, I'm open to changes, this particular implementation isn't as important as the feature itself - per request caching
• 🔌 This should apply to all integrations
• 🕷 This is not a bug fix
• 📖 New test added for this behaviour
• ✏️ This PR adds an overridable method protected shouldCacheRequest(request: Request): boolean to RESTDataSource, allowing users to configure which requests are held in the http cache. This is useful for query-like POST requests, for example, or if users want to prevent certain GET requests from ever being cached.

Usage

Users extending from RESTDataSource may override the default implementation of shouldCacheRequest:

class MyDataSource extends RESTDataSource {
  protected shouldCacheRequest(request: Request): boolean {
    const { pathname } = new URL(request.url);
    switch (pathname) {
      case '/query-post':
      case '/definitely-cache-this':
        return true;
      case '/very-dynamic-get-request':
        return false;
      default:
        return request.method === 'GET';
    }
  }
}

[glasser]

Just want to note that reviewing this is definitely on my radar. That said, new features often require more care in review than bug fixes, and while I'm the lead Apollo Server maintainer, I am not super familiar with the internals of RESTDataSource, so reviewing this is blocked on finding time to understand the package more deeply.

[bennypowers]

Thanks for the reply! We've been trying to leverage more of the advanced caching and deduplication features, and doing some code spelunking lately in these files. If you have any questions about our use case or how we see this feature helping, lmk.

[jondmcelroy]

Would love to have this merged, I was just about to work on making a PR for it, so I am glad that I checked first.
@glasser This is a very small change which is just moving some code around to make it more accessible for people to work with the memoized code. Right now we have to copy the whole fetch function to extend this.

[glasser]

I don't believe anyone currently on the Apollo Server team has come up to speed enough yet on apollo-datasource-rest to be able to evaluate something that changes its caching policies. eg, I am vaguely aware that the software actually has two levels of caching (one that's always in-memory and per-request, and one that uses a shared cache) and I'd want to understand whether a feature like "shouldCacheRequest" should affect both of these caches or just one.

Perhaps apollo-datasource-http (https://github.com/StarpTech/apollo-datasource-http) solves this problem better? Or there might be a caching HTTP client for Node out there these days that isn't tangentially linked to a GraphQL server; I know there wasn't such a thing when apollo-datasource-rest was created but maybe there is now.

[jondmcelroy]

@glasser
This PR does not change the current functionality from what is already there. The protected shouldCacheRequest is there to allow overwriting in child classes (of which there are none in this repo). Its just to allow extensibility.

My use case for example is extending it to also cache posts that are graphql calls instead of only caching gets.

[glasser]

Yes, but it seems fraught to name a new entry point "should cache request" without a thorough understanding of the two layers of caching in the software and whether it's an appropriate name for whichever level it affects.

[jkbailey]

I am also looking for this pull request to be merged. This allows us to extend the current cacheing mechanism and allows us to write custom logic around cacheing. Any way I can help get this merged in @glasser ?

[glasser]

At the moment, your best bet would be to find a Node caching HTTP client with a maintainer.

apollo-datasource-rest seems like a popular and cool piece of code, and I'd love to learn about it and maintain it better in theory, but maintaining the GraphQL server in this repository has been enough work for myself and the other members of the current Apollo Server team that none of us have found the time to learn about the tangentially related Node caching HTTP client that happens to be in the same repository.

[glasser]

This package has moved to a new repository. I'm closing this PR, but we do plan to spend some time working on RESTDataSource soon; I opened apollographql/datasource-rest#64 to remind us to look at this PR. Sorry for the delay!