Apollo Server 2.0 - Caching #1163

martijnwalraven · 2018-06-12T21:01:48Z

This PR adds a first version of an apollo-server-caching package that will be used by the new Data Source API. It uses jest for testing, which means that its tests don't yet run as part of CI.

In addition to the new package, this PR attempts to fix types for fetch and related classes like Request and Response. Previously, the lib entry in tsconfig.json contained dom, which brings in a lot of types that don't make sense in server environments. I tried using webworker instead, but that also doesn't accurately represent the actual environments we'll be running in. In addition, the types we do use from those declarations contained various omissions and mistakes.

To fix this, I've added a top-level types directory with slimmed down declarations for fetch and related types, as well as the WHATWG URL and URLSearchParams (which are used in Data Sources). These are all available in global scope on Cloudflare Workers and fly.io. On Node, this requires polyfills that for now I've included in apollo-server-caching. We may want to move these to apollo-server-core or extract them into a separate package. I'm not sure what the best approach is, so suggestions are very welcome.

See http://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-9.html

evans

This is a great start! We could probably merge now, so that we have an api that people can start playing with. Without some documentation, I hesitate to hit the button. I'll attempt to put together some thing for your morning.

My biggest worry right now is that the caching does not occur cross-request, since a new cache is created during each request. Also I didn't see a place for the user to pass in their own cache, which will soon be possible in #1169. Completely understand the hesitancy, since the api's have been changing so much

evans · 2018-06-14T20:42:07Z

packages/apollo-datasource-rest/src/RESTDataSource.ts

+export abstract class RESTDataSource<TContext = any> {
+  abstract baseURL: string;
+
+  httpCache!: HTTPCache;


I generally like variable scopes explicit, public in this case.

evans · 2018-06-14T20:48:36Z

packages/apollo-server-core/src/runHttpQuery.ts

+        const httpCache = new HTTPCache();
+
+        for (const dataSource of Object.values(dataSources)) {
+          dataSource.httpCache = httpCache;


This explicit coupling of data sources to an httpCache is worrisome. Maybe a lifecycle method, such as willReceiveCache or willReceiveContext might be a bit better. In addition to the httpCache, the primitive cache should be an argument.

The other option would be to provide them as arguments to the dataSources api. Also I'm worried that these methods are called for each request, so with the current setup, it doesn't appear that there would be any caching, since the HttpCache is created each time

evans · 2018-06-14T20:55:18Z

packages/apollo-datasource-rest/src/RESTDataSource.ts

+
+export type Params = { [name: string]: any };
+
+export abstract class RESTDataSource<TContext = any> {


Awesome! We'll eventually create an interface to share between DataSources. Starting with a concrete use case is definitely the way to go.

evans · 2018-06-15T00:22:57Z

Starting to write some tests. It might be nice to have the context value be sources instead of dataSources, since I got confused by the camel case and there wasn't a warning

evans · 2018-06-15T00:25:37Z

It appears that we require that a rest response be parsable json. If a rest endpoint returns a string, then we should treat it as such

glasser

This looks nice!

That said, the examples so far don't really make use of the fact that each DataSource is attached to an incoming request (created anew) — not clear to me that it gives you anything you don't get just from import { id } from './datasources. Presumably there are plans here, like integrating DataLoader, tracing, auth/context, etc? Would be nice to see at least one of those things existing in practice before 2.0 release.

glasser · 2018-06-15T01:53:05Z

packages/apollo-server-core/src/ApolloServer.ts

@@ -121,6 +122,10 @@ export class ApolloServerBase {
      delete requestOptions.persistedQueries;
    }

+    if (!requestOptions.cache) {


I think we should stick to the philosophy that all extra features can be turned off with false.

glasser · 2018-06-15T01:58:22Z

packages/apollo-server-env/src/polyfills/fetch.ts

@@ -0,0 +1,114 @@
+import fetch, { Request, Response, Headers } from 'node-fetch';
+
+Object.assign(global, { fetch, Request, Response, Headers });


Should this be conditional, or ideally not actually global? I think it would be cleaner to have code explicitly import these symbols from apollo-server-env. I know I would want to be able to find (while reading say RESTDataSource) where that URL and Request symbol come from...

I've been going back and forth on this, so definitely open to changing it. The reason for making them global was that this is how environments other than Node (browser, Cloudflare, fly.io) expose them.

We also have existing code (including apollo-engine-reporting) relying on the types being global (because it compiles with the dom lib), so this would require those to also import from apollo-server-env.

glasser · 2018-06-15T01:58:33Z

packages/apollo-server-env/src/polyfills/url.ts

@@ -0,0 +1,47 @@
+import { URL, URLSearchParams } from 'url';
+
+Object.assign(global, { URL, URLSearchParams });


Should this be conditional, or ideally not actually global?

glasser · 2018-06-15T02:18:28Z

packages/apollo-datasource-rest/package.json

+  "dependencies": {
+    "apollo-server-env": "2.0.0-beta.7",
+    "http-cache-semantics": "^4.0.0",
+    "lru-cache": "^4.1.3"


We're using quick-lru for PQs — unless there's a compelling reason we should try to use the same one for both.

lru-cache supports max size based on the size of the items in it (not just the number of items), so that seemed important for a response cache.

glasser · 2018-06-15T02:27:54Z

packages/apollo-datasource-rest/src/RESTDataSource.ts

+    );
+  }
+
+  private async fetch(


seems like this might as well be protected if you wanted to be able to call it with a calculated method

glasser · 2018-06-15T02:46:00Z

packages/apollo-datasource-rest/src/HTTPCache.ts

+    response: Response,
+    policy: CachePolicy,
+  ): Promise<Response> {
+    if (!response.headers.has('Cache-Control') || !policy.storable())


This first clause deserves a comment. Specifically, this means we are not caching the "cacheable by default" response types like 200 based on a heuristic based on its last-modified. Seems reasonable but worth documenting.

It also means we don't support the somewhat older "expires" header, which maybe was an oversight?

glasser · 2018-06-15T02:50:13Z

packages/apollo-datasource-rest/src/RESTDataSource.ts

+      }
+      const response = await this.httpCache.fetch(request, init);
+      if (response.ok) {
+        return response.json();


So several times while reading this code I've been confused by a fact about async functions that perhaps I should just internalize more strongly: you can return a Promise explicitly from them, or return an expression that is calling another async function, without having to explicitly await, because nested Promises are unwrapped.

As far as I understand, in an async function f1, calling another async function f2 like return f2() and return await f2() are identical. But I definitely felt odd reading this function and thinking "ok, you have to await response.text() but not response.json()? that's weird!".

Would it be reasonable to consistently use await when calling nested async functions (assuming you aren't actually trying to manipulate the Promise of course) or should I just learn the language?

My mental model for this is that you're always returning a promise from an async function, and with that returning the result of another async function doesn't seem surprising. Also not entirely sure if the generated code with an explicit await is identical, although I'd expect it to be optimized away. We may want to see what conventions other people have adopted.

glasser · 2018-06-15T02:50:58Z

packages/apollo-datasource-rest/src/RESTDataSource.ts

+      }
+      const response = await this.httpCache.fetch(request, init);
+      if (response.ok) {
+        return response.json();


Also @evans has a much more concrete point: we should be able to use this method to fetch non-JSON results as well.

glasser · 2018-06-15T02:52:23Z

packages/apollo-datasource-rest/src/HTTPCache.ts

+
+    const cacheKey = cacheKeyFor(request);
+
+    const body = await response.text();


These lines assume that you are always getting text/UTF8 and not say some binary protocol.

quick-lru or keyv (I forget which one) uses https://www.npmjs.com/package/json-buffer which is nice I think.

glasser · 2018-06-15T03:04:07Z

packages/apollo-datasource-rest/src/HTTPCache.ts

+
+    if (policy.satisfiesWithoutRevalidation(policyRequestFrom(request))) {
+      const headers = policy.responseHeaders();
+      return new Response(body, { status: policy._status, headers });


Instead of depending on this internal detail, just save status separately in our cache alongside policy and body. (There's another case of it being used below; I'm pretty sure that revalidatedPolicy._status is always equal to policy._status so you can just use the same saved value.)

martijnwalraven · 2018-06-15T05:15:05Z

packages/apollo-datasource-rest/src/RESTDataSource.ts

-  willSendRequest?(request: Request): void;
+  public willSendRequest?(request: Request): void;
+
+  public willReceiveCache(httpCache: HTTPCache) {


Hmmm, not sure I understand the benefits of this. It will actually open up users to mistakes when they override these methods and forget to call super (or set the properties themselves).

If the idea is that we'd like to have a lifecycle method that's called when the datasource is fully initialized, we should probably define it separately and call it when both cache and context have been set.

That makes sense! We don't want users to mess with those "lifecycles", then we should probably have a ready or willReceiveRequest lifecycle for after the httpCache and context are set.

evans · 2018-06-15T05:23:30Z

packages/apollo-datasource-rest/src/RESTDataSource.ts

+
+  public willSendRequest?(request: Request): void;
+
+  public willReceiveCache(httpCache: HTTPCache) {


@glasser pointed out that these names aren't the best, since the data source is really just receiving the cache, so something like receiveCache would probably be better

I'm not convinced we need these at all, but if we do I like the consistency with React's willReceiveProps.

want to make sure #1163 (comment) doesn't get buried

evans force-pushed the version-2 branch from 5770674 to afcd170 Compare June 13, 2018 06:11

martijnwalraven force-pushed the server-2.0/caching branch from 081abe1 to edbb2c8 Compare June 14, 2018 07:30

martijnwalraven added 5 commits June 14, 2018 20:17

Enable declarationMap in tsconfig.json

8f74518

See http://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-9.html

Add apollo-server-caching package and improve typings

1c23535

Remove superfluous test steps

f8658b9

Add .npmignore to apollo-server-caching

4f25000

Add apollo-server-env and apollo-datasource-rest packages

395da0f

martijnwalraven force-pushed the server-2.0/caching branch from edbb2c8 to 395da0f Compare June 14, 2018 18:18

martijnwalraven added 2 commits June 14, 2018 20:21

Fix broken imports

dbd8288

Use prepublish instead of prepare

eda8ce9

evans reviewed Jun 14, 2018

View reviewed changes

Merge branch 'version-2' into server-2.0/caching

80d074f

evans added 3 commits June 14, 2018 17:42

cache is now passed to data sources from ApolloServer constructor

e6aae1d

fix Object.values to use the object passed in rather than this

29d5998

add initial datasource test

7b2d694

glasser reviewed Jun 15, 2018

View reviewed changes

evans added 3 commits June 14, 2018 20:15

docs: initial data source documentation

b39db20

docs: initial data source documentation

93d21f8

compiles and documentation now highlights code in data-sources.md

6548196

martijnwalraven commented Jun 15, 2018

View reviewed changes

evans reviewed Jun 15, 2018

View reviewed changes

Some changes to the data source docs

1703e27

evans added this to the Release 2.0 milestone Jun 15, 2018

evans merged commit 685d339 into version-2 Jun 15, 2018

evans mentioned this pull request Jun 21, 2018

Apollo Server 2.0 #1125

Merged

abernix deleted the server-2.0/caching branch August 10, 2018 16:47

dependencies bot mentioned this pull request Aug 15, 2018

Update apollo-server-express in / from 1.2.0 to 2.0.2 LibreTubeApp/LibreTube#158

Open

github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apollo Server 2.0 - Caching #1163

Apollo Server 2.0 - Caching #1163

martijnwalraven commented Jun 12, 2018

evans left a comment

evans Jun 14, 2018

evans Jun 14, 2018

evans Jun 14, 2018

evans Jun 14, 2018

evans commented Jun 15, 2018

evans commented Jun 15, 2018

glasser left a comment

glasser Jun 15, 2018

glasser Jun 15, 2018

martijnwalraven Jun 15, 2018

glasser Jun 15, 2018

glasser Jun 15, 2018

martijnwalraven Jun 15, 2018

glasser Jun 15, 2018

glasser Jun 15, 2018

glasser Jun 15, 2018

martijnwalraven Jun 15, 2018

glasser Jun 15, 2018

glasser Jun 15, 2018

glasser Jun 15, 2018

martijnwalraven Jun 15, 2018

evans Jun 15, 2018

evans Jun 15, 2018

martijnwalraven Jun 15, 2018

evans Jun 15, 2018


		export type Params = { [name: string]: any };

		export abstract class RESTDataSource<TContext = any> {

		@@ -0,0 +1,114 @@
		import fetch, { Request, Response, Headers } from 'node-fetch';

		Object.assign(global, { fetch, Request, Response, Headers });

		@@ -0,0 +1,47 @@
		import { URL, URLSearchParams } from 'url';

		Object.assign(global, { URL, URLSearchParams });


		const cacheKey = cacheKeyFor(request);

		const body = await response.text();


		public willSendRequest?(request: Request): void;

		public willReceiveCache(httpCache: HTTPCache) {

Apollo Server 2.0 - Caching #1163

Apollo Server 2.0 - Caching #1163

Conversation

martijnwalraven commented Jun 12, 2018

evans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evans commented Jun 15, 2018

evans commented Jun 15, 2018

glasser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment