feat(response-cache): add extras property to get scope and its metadata from buildResponseCacheKey by k-i-k-s · Pull Request #2202 · graphql-hive/envelop

k-i-k-s · 2024-04-02T16:15:33Z

Issue related : #2181

Description

Include a new extras property to get scope (as a Proxy to avoid performance impact when unused) in buildResponseCacheKey, allowing more granularity over cache control.

Type of change

New feature (non-breaking change which adds functionality)
This change requires a documentation update

How Has This Been Tested?

I added some unit tests to check that PRIVATE scope is correctly retrieved from a query, when directive @cacheControl(scope: PRIVATE) is put on query, field and subfield.

Checklist:

I have followed the
CONTRIBUTING doc and the
style guidelines of this project
I have performed a self-review of my own code
I have commented on my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

changeset-bot · 2024-04-02T16:15:37Z

🦋 Changeset detected

Latest commit: 9e8ea70

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@envelop/response-cache	Minor
@envelop/response-cache-cloudflare-kv	Major

Not sure what this means? Click here to learn what changesets are.

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

…ta from buildResponseCacheKey

k-i-k-s · 2025-10-29T15:27:58Z

Hi @ardatan, I'm back with a new proposal for this scope functionality with some improvements.
Tell me what you think about those changes!

ardatan · 2025-10-29T17:11:42Z

@EmrysMyrddin Could you take a look?

EmrysMyrddin · 2025-10-29T19:30:54Z

+            new Proxy(
+              {
+                documentString: getDocumentString(onExecuteParams.args),
+                variableValues: onExecuteParams.args.variableValues,
+                operationName: onExecuteParams.args.operationName,
+                sessionId,
+                context: onExecuteParams.args.contextValue,
+                extras: undefined as any,
+              },
+              {
+                get: (obj, prop) => {
+                  if (prop === 'extras') {
+                    return getScopeFromQuery(schema, onExecuteParams.args.document.loc.source.body);
+                  }
+                  return obj[prop as keyof typeof obj];
+                },
+              },
+            ),


I don't remembre how it was implemented before, but is there a reason to use a proxy instead of classic getters?

Like this:

Suggested change

new Proxy(

{

documentString: getDocumentString(onExecuteParams.args),

variableValues: onExecuteParams.args.variableValues,

operationName: onExecuteParams.args.operationName,

sessionId,

context: onExecuteParams.args.contextValue,

extras: undefined as any,

},

{

get: (obj, prop) => {

if (prop === 'extras') {

return getScopeFromQuery(schema, onExecuteParams.args.document.loc.source.body);

}

return obj[prop as keyof typeof obj];

},

},

),

{

documentString: getDocumentString(onExecuteParams.args),

variableValues: onExecuteParams.args.variableValues,

operationName: onExecuteParams.args.operationName,

sessionId,

context: onExecuteParams.args.contextValue,

get extras() {

return getScopeFromQuery(schema, onExecuteParams.args.document.loc.source.body);

},

},

A recent habit to use proxies for their flexibility, but I can see that a getter in this case would fit better for clarity sake.

EmrysMyrddin · 2025-10-29T19:33:29Z

+    return { scope: 'PUBLIC' as const, metadata: {} };
+  });
+
+  return fn({ query });


This will disallow memoization isn't it ? Since { query } creates a new object each time the function is called.

And I'm not sure to understand why schema is not part of the memoize1 key ? Because I fear that on schema update, you will get outadated metadata for the same query.

Fair points!
I'll include schema using memoize2 function and stringify those objects to fix comparison.

EmrysMyrddin · 2025-10-29T19:41:44Z

+export let schema: GraphQLSchema;
+let ttlPerSchemaCoordinate: Record<string, CacheControlDirective['maxAge']> = {};
+let scopePerSchemaCoordinate: Record<string, CacheControlDirective['scope']> = {};


Why do you extract those outside of the plugin? I feel that it will break things, because multiple instances of the same plugin would share those variables.

EmrysMyrddin · 2025-10-29T19:56:58Z

+export function isPrivate(
+  typeName: string,
+  data?: Record<string, NonNullable<CacheControlDirective['scope']>>,
+): boolean {
+  if (scopePerSchemaCoordinate[typeName] === 'PRIVATE') {
+    return true;
+  }
+  return data
+    ? Object.keys(data).some(
+        fieldName => scopePerSchemaCoordinate[`${typeName}.${fieldName}`] === 'PRIVATE',
+      )
+    : false;
+}


Why do you have to make a duplicate of this function outside of the plugin?

Mb I merged local code I made last year with the one I use today and simply forgot there was already this function.
I'll keep the exported one since it's needed in get-scope.ts file.

… better scope memoization

k-i-k-s · 2025-10-30T11:18:53Z

Hi @EmrysMyrddin, how's it going today ?

I made updates to fix the points you brought up :

extras is a function now and the schema is its parameter
no more exported schema since it's a param of extras
memoization fixed with a combination of WeakMap (schema as the key) containing an LRU cache per schema (using query as key)
also added hitCache boolean property in metadata and updated tests accordingly

…nd sizePerSchema is accessible

k-i-k-s · 2025-11-19T11:32:49Z

It seems square now with :

options parameter accessible to include metadata and alter size per schema in LRUCache
changeset, unit tests and README updated

Tell me if there's still something blocking the merge @EmrysMyrddin.

Take care !

EmrysMyrddin · 2025-12-17T21:15:45Z

Hi @k-i-k-s, would you mind allowing modification of your PR by maintainers?

I've taken a closer look and addressed issues so that we can finally merge this and handle the use case explained in the linked issue.

You can take a look at what it looks like here: #2815

If you prefer not to give write access to your PR, I can merge mine instead.

k-i-k-s · 2025-12-17T22:14:20Z

Hi, I don't think I have permission to give write access to the fork, merge your own branch I don't mind, it'll be far easier.
Good to see you're continuing implementation of this feature !
Is there an internal need on gql hive for you to personally intervene ?

EmrysMyrddin · 2025-12-22T15:36:14Z

Ok, then don't hesitate to give my PR a try 🙂

No, there isn't any new internal need for the feature, but I'm in the process of merging Envelop and Yoga repository together to ease development.

So before this can happen, we need to merge all PRs we don't want to loose. And I didn't want to let this one behind given the effort you put into it 🙂

k-i-k-s force-pushed the feat/response-cache-query-scope branch 6 times, most recently from 4f30aba to a5468bb Compare April 3, 2024 16:50

k-i-k-s force-pushed the feat/response-cache-query-scope branch from a5468bb to 9e8ea70 Compare April 17, 2024 13:32

feat(response-cache): add extras property to get scope and its metada…

c4de9af

…ta from buildResponseCacheKey

k-i-k-s force-pushed the feat/response-cache-query-scope branch from 9e8ea70 to c4de9af Compare October 29, 2025 14:49

k-i-k-s changed the title ~~feat(response-cache): added getScope callback in buildResponseCacheKey~~ feat(response-cache): add extras property to get scope and its metadata from buildResponseCacheKey Oct 29, 2025

ardatan requested a review from EmrysMyrddin October 29, 2025 17:11

EmrysMyrddin reviewed Oct 29, 2025

View reviewed changes

feat(response-cache): extras as a function with schema as a param and…

d4ab39b

… better scope memoization

feat(response-cache): extras metadata uses includeExtensionMetadata a…

839204e

…nd sizePerSchema is accessible

EmrysMyrddin mentioned this pull request Dec 17, 2025

feat(response-cache): shared key for public responses #2815

Open

Uh oh!

Conversation

k-i-k-s commented Apr 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Checklist:

Uh oh!

changeset-bot Bot commented Apr 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

k-i-k-s commented Oct 29, 2025

Uh oh!

ardatan commented Oct 29, 2025

Uh oh!

EmrysMyrddin Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k-i-k-s Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

EmrysMyrddin Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

EmrysMyrddin Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

k-i-k-s Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

EmrysMyrddin Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

EmrysMyrddin Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

k-i-k-s Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

k-i-k-s commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k-i-k-s commented Nov 19, 2025

Uh oh!

EmrysMyrddin commented Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k-i-k-s commented Dec 17, 2025

Uh oh!

EmrysMyrddin commented Dec 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

k-i-k-s commented Apr 2, 2024 •

edited

Loading

changeset-bot Bot commented Apr 2, 2024 •

edited

Loading

EmrysMyrddin Oct 29, 2025 •

edited

Loading

k-i-k-s commented Oct 30, 2025 •

edited

Loading

EmrysMyrddin commented Dec 17, 2025 •

edited

Loading