Review or Edit in CodeSandbox

Open the branch in Web Editor • VS Code • Insiders

Open Preview

cc @ardatan

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

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

I'll take a crack at it, sure.

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.

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?

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?

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 👍

@ardatan apologies looks like I had some prettier formatting issues with my initial change. I've ran prettier and I think it should pass all your PR checks 🤞

@ardatan looks like load test failed on node 21, and the e2e tests timed out in node 20. These don't appear to be related to my code changes. lmk if there is anything more you'd like me to do to facilitate here.

Hey @mmadson, I've fixed the flaky tests! Sorry about that. Can you please rebase on master?

We can merge it now! Thanks for the PR @mmadson !