✨ Allowing extraCacheKeyData to provide a cache key prefix

[kschrade]

I have tests as well in a local branch

[changeset-bot]

⚠️ No Changeset found

Latest commit: 28d2462

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[netlify]

✅ Deploy Preview for apollo-server-docs canceled.

Built without sensitive environment variables

Name	Link
🔨 Latest commit	28d2462
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-server-docs/deploys/62c71588f8731e00097d3e2e

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 28d2462:

Sandbox	Source
Apollo Server Typescript	Configuration
Apollo Server	Configuration

[glasser]

Hmm, this seems even more over-specific than #6428. Why not just let you completely override the cacheKeyString function with an arbitrary (key: CacheKey, requestContext: GraphQLRequestContext<TContext>): Promise<string> function?

I suppose one answer to my question is that this would make CacheKey part of the API rather than an internal implementation detail. So maybe my idea isn't great.

But I still don't see why hardcoding the concept of "dynamic prefix" into the API makes sense. Maybe some other cache system would work better with a dynamic suffix. So if we don't want to export CacheKey, maybe we leave extraCacheKeyData alone, and we instead add a cacheKeyFromHash(requestContext: GraphQLRequestContext<TContext>, hash: string): Promise<string>? I just really feel like "prefix" is overfitting the solution to today's particular request.

(BTW I'd prefer Promise over ValueOrPromise; we removed ValueOrPromise from the main AS plugin API in AS3 and are removing it from this plugin's configuration in AS4.)

[kschrade]

Hmm, this seems even more over-specific than #6428. Why not just let you completely override the cacheKeyString function with an arbitrary (key: CacheKey, requestContext: GraphQLRequestContext): Promise function?

Don't mind that solution either! The only thing we have to be ok with creating a lot more promises.

I suppose one answer to my question is that this would make CacheKey part of the API rather than an internal implementation detail. So maybe my idea isn't great.

I think it's fine, it allows people to make their own cache keys. If they don't want to fall back on what is there today.

But I still don't see why hardcoding the concept of "dynamic prefix" into the API makes sense. Maybe some other cache system would work better with a dynamic suffix. So if we don't want to export CacheKey, maybe we leave extraCacheKeyData alone, and we instead add a cacheKeyFromHash(requestContext: GraphQLRequestContext, hash: string): Promise? I just really feel like "prefix" is overfitting the solution to today's particular request.

I'm game for that. I think making it not a promise would be a good idea though. If we allow for it to be a promise we're opening the gates to doing a lot of stuff that can cause problems. IE "let's query our DB to see what key to give this?!"

[glasser]

Latest suggestion based on chat with @kschrade :

generateCacheKey?(requestContext: GraphQLRequestContext<TContext>, keyData: unknown): string

The assumption is that keyData is something for which JSON.stringify manages to capture all relevant data (but if you want to use a different JSON implementation, or something other than JSON, that's fine too). You can also swap out the hash if you want!

@kschrade and I don't see a particular use case for this to be async so for potential performance reasons let's keep this sync.

However let's not make this replace extraCacheKeyData because we want to call that only once even if we end up constructing the key multiple times (once with session ID, once without).

[glasser]

note btw that if we later decide to export CacheKey, changing the signature to a more specific keyData type will be backwards compatible, in that you can pass a function taking unknown in for a type now declared to be functions taking CacheKey.

[kschrade]

closing for other pr