fix context undefined in useResponseCache ifDef enabled fn

[mmadson]

🚨 IMPORTANT: Please do not create a Pull Request without creating an issue first.

Any change needs to be discussed before proceeding. Failure to do so may result in the rejection of the pull request.

Description

Conntext not available when evaluating responseCache if function. Reason is that javascript function constructor does not inherit lexical scope. Instead we must pass the context as a named arg.

Fixes #6925

Type of change

Please delete options that are not relevant.

• [X] Bug fix (non-breaking change which fixes an issue)
• [ ] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
• [ ] This change requires a documentation update

Screenshots/Sandbox (if appropriate/relevant):

Adding links to sandbox or providing screenshots can help us understand more about this PR and take action on it as appropriate

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

• [ ] Test A
• [ ] Test B

Test Environment:

• OS:
• @graphql-mesh/...:
• NodeJS:

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 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, and I have added a changeset using yarn changeset that bumps the version
• [ ] New and existing unit tests and linter rules pass locally with my changes
• [ ] Any dependent changes have been merged and published in downstream modules

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

[codesandbox[bot]]

Review or Edit in CodeSandbox

Open the branch in Web Editor • VS Code • Insiders
Open Preview

[mmadson]

cc @ardatan

[ardatan]

Thanks for the PR! Do you think we can add tests to prevent future regressions?

[mmadson]

Thanks for the PR! Do you think we can add tests to prevent future regressions?

I'll take a crack at it, sure.

[mmadson]

Thanks for the PR! Do you think we can add tests to prevent future regressions?

@ardatan The response-cache plugin does not have a tests folder or any tests defined currently. I attempted to copy the tests from the rate-limit plugin and adapt them to response cache but quickly encountered setup issues. When trying to pass the response-cache plugin to the envelop factory method, I got an error saying that the response-cache plugin must be used with graphql-yoga. So I'm not sure the best way to create a test harness. If you can help lay the groundwork here I'd be happy to contribute a unit test outlining the expectation for the request context being accessible in the ifdef setting.

I'm currently manually testing this with the help of the patch package lib

• https://www.npmjs.com/package/patch-package

Here is the diff

diff --git a/node_modules/@graphql-mesh/plugin-response-cache/cjs/index.js b/node_modules/@graphql-mesh/plugin-response-cache/cjs/index.js
index 5c98d77..a4acb6a 100644
--- a/node_modules/@graphql-mesh/plugin-response-cache/cjs/index.js
+++ b/node_modules/@graphql-mesh/plugin-response-cache/cjs/index.js
@@ -20,7 +20,7 @@ function generateSessionIdFactory(sessionIdDef) {
 function generateEnabledFactory(ifDef) {
     return function enabled(context) {
         // eslint-disable-next-line no-new-func
-        return new Function(`return ${ifDef}`)();
+        return new Function("context", `return ${ifDef}`)(context);
     };
 }
 function getBuildResponseCacheKey(cacheKeyDef) {
diff --git a/node_modules/@graphql-mesh/plugin-response-cache/esm/index.js b/node_modules/@graphql-mesh/plugin-response-cache/esm/index.js
index 94f1947..3f0641f 100644
--- a/node_modules/@graphql-mesh/plugin-response-cache/esm/index.js
+++ b/node_modules/@graphql-mesh/plugin-response-cache/esm/index.js
@@ -18,7 +18,7 @@ function generateSessionIdFactory(sessionIdDef) {
 function generateEnabledFactory(ifDef) {
     return function enabled(context) {
         // eslint-disable-next-line no-new-func
-        return new Function(`return ${ifDef}`)();
+        return new Function("context", `return ${ifDef}`)(context);
     };
 }
 function getBuildResponseCacheKey(cacheKeyDef) {

with this diff in place and patch package installed, you can effectively modify the built library and observe the changed behavior. Before the patch my ifdef function fails when I reference context request headers and after the patch is applied -- no issues.

[mmadson]

Thanks for the PR! Do you think we can add tests to prevent future regressions?

@ardatan The response-cache plugin does not have a tests folder or any tests defined currently. I attempted to copy the tests from the rate-limit plugin and adapt them to response cache but quickly encountered setup issues. When trying to pass the response-cache plugin to the envelop factory method, I got an error saying that the response-cache plugin must be used with graphql-yoga. So I'm not sure the best way to create a test harness. If you can help lay the groundwork here I'd be happy to contribute a unit test outlining the expectation for the request context being accessible in the ifdef setting.

I'm currently manually testing this with the help of the patch package lib

• https://www.npmjs.com/package/patch-package

Here is the diff

diff --git a/node_modules/@graphql-mesh/plugin-response-cache/cjs/index.js b/node_modules/@graphql-mesh/plugin-response-cache/cjs/index.js
index 5c98d77..a4acb6a 100644
--- a/node_modules/@graphql-mesh/plugin-response-cache/cjs/index.js
+++ b/node_modules/@graphql-mesh/plugin-response-cache/cjs/index.js
@@ -20,7 +20,7 @@ function generateSessionIdFactory(sessionIdDef) {
 function generateEnabledFactory(ifDef) {
     return function enabled(context) {
         // eslint-disable-next-line no-new-func
-        return new Function(`return ${ifDef}`)();
+        return new Function("context", `return ${ifDef}`)(context);
     };
 }
 function getBuildResponseCacheKey(cacheKeyDef) {
diff --git a/node_modules/@graphql-mesh/plugin-response-cache/esm/index.js b/node_modules/@graphql-mesh/plugin-response-cache/esm/index.js
index 94f1947..3f0641f 100644
--- a/node_modules/@graphql-mesh/plugin-response-cache/esm/index.js
+++ b/node_modules/@graphql-mesh/plugin-response-cache/esm/index.js
@@ -18,7 +18,7 @@ function generateSessionIdFactory(sessionIdDef) {
 function generateEnabledFactory(ifDef) {
     return function enabled(context) {
         // eslint-disable-next-line no-new-func
-        return new Function(`return ${ifDef}`)();
+        return new Function("context", `return ${ifDef}`)(context);
     };
 }
 function getBuildResponseCacheKey(cacheKeyDef) {

with this diff in place and patch package installed, you can effectively modify the built library and observe the changed behavior. Before the patch my ifdef function fails when I reference context request headers and after the patch is applied -- no issues.

ping @ardatan , thoughts on the above?

[ardatan]

It seems integration tests are not failing which is ok for now. Let's merge this, then we can think of individual unit tests later on. Thanks for the PR! At least, could you create a changeset by using yarn changeset so we can bump the version and release?

[mmadson]

It seems integration tests are not failing which is ok for now. Let's merge this, then we can think of individual unit tests later on. Thanks for the PR! At least, could you create a changeset by using yarn changeset so we can bump the version and release?

changeset complete 👍