-
Notifications
You must be signed in to change notification settings - Fork 332
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix context undefined in useResponseCache ifDef enabled fn #6927
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
cc @ardatan |
Thanks for the PR! |
I'll take a crack at it, sure. |
@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 Here is the diff
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. |
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 ! |
🚨 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.
expected)
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 Environment:
@graphql-mesh/...
:Checklist:
CONTRIBUTING doc and the
style guidelines of this project
changeset using
yarn changeset
that bumps the versionFurther 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...