Skip to content
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

Response Cache plugin with small changes for better type-safety #1359

Merged
merged 7 commits into from
Jul 28, 2022

Conversation

ardatan
Copy link
Collaborator

@ardatan ardatan commented Jul 6, 2022

See the changesets
https://github.com/dotansimha/graphql-yoga/pull/1359/files

I am open for discussions about passing params and request instead of context in sessionId factory.

We need some unit tests for the new sessionId factory and to make sure execute, validate or even parse are never called before this plugin returns the cached response.

Need docs

@changeset-bot
Copy link

changeset-bot bot commented Jul 6, 2022

🦋 Changeset detected

Latest commit: a0026b7

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

This PR includes changesets to release 2 packages
Name Type
@graphql-yoga/common Minor
@graphql-yoga/plugin-response-cache 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

@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2022

✅ Benchmark Results

     ✓ no_errors
     ✓ expected_result

     checks.........................: 100.00% ✓ 112576      ✗ 0    
     data_received..................: 15 MB   499 kB/s
     data_sent......................: 6.5 MB  216 kB/s
     http_req_blocked...............: avg=2.12µs   min=1.2µs   med=1.7µs   max=2.74ms  p(90)=2.29µs  p(95)=2.66µs  
     http_req_connecting............: avg=32ns     min=0s      med=0s      max=1.84ms  p(90)=0s      p(95)=0s      
   ✓ http_req_duration..............: avg=416.2µs  min=253.8µs med=365.1µs max=21.95ms p(90)=476.2µs p(95)=586.2µs 
       { expected_response:true }...: avg=416.2µs  min=253.8µs med=365.1µs max=21.95ms p(90)=476.2µs p(95)=586.2µs 
     http_req_failed................: 0.00%   ✓ 0           ✗ 56288
     http_req_receiving.............: avg=29.52µs  min=16.2µs  med=24.7µs  max=9.28ms  p(90)=35.9µs  p(95)=41.2µs  
     http_req_sending...............: avg=12.09µs  min=6.1µs   med=8.3µs   max=4.87ms  p(90)=19µs    p(95)=20.5µs  
     http_req_tls_handshaking.......: avg=0s       min=0s      med=0s      max=0s      p(90)=0s      p(95)=0s      
     http_req_waiting...............: avg=374.58µs min=226.4µs med=328.1µs max=21.83ms p(90)=425.4µs p(95)=536.57µs
     http_reqs......................: 56288   1876.177746/s
     iteration_duration.............: avg=525.69µs min=322.1µs med=472.6µs max=24.19ms p(90)=601.6µs p(95)=709.9µs 
     iterations.....................: 56288   1876.177746/s
     vus............................: 1       min=1         max=1  
     vus_max........................: 1       min=1         max=1  

@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2022

The latest changes of this PR are not available as canary, since there are no linked changesets for this PR.

@ardatan ardatan changed the title WIP: Response Cache plugin Response Cache plugin Jul 6, 2022
@ardatan ardatan marked this pull request as ready for review July 6, 2022 17:49
const operationIdByRequest = new WeakMap<Request, string>()

// We trick Envelop plugin by passing operationId as sessionId so we can take it from cache key builder we pass to Envelop
function sessionFactoryForEnvelop({ request }: YogaInitialContext) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we don't have an access to the context from buildResponseCacheKey

@ardatan ardatan changed the title Response Cache plugin Response Cache plugin with small improvements in type-safety Jul 7, 2022
@ardatan ardatan changed the title Response Cache plugin with small improvements in type-safety Response Cache plugin with small changes for better type-safety Jul 7, 2022
@ardatan ardatan requested a review from n1ru4l July 7, 2022 09:54
Copy link
Collaborator

@n1ru4l n1ru4l left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New envelop response-cache plugin is released, I think you can use it and then also change the target branch here to v3.

Comment on lines 55 to 50
const operationId = await buildResponseCacheKey({
documentString: params.query!,
variableValues: params.variables,
operationName: params.operationName,
sessionId: options.session(params, request),
})
const cachedResponse = await cache.get(operationId)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for mutation and subscription operations we don't really need to lookup in the cache. However, we would need to already parse the string in order to reliably detect whether the operation is a mutation 🤔.
Maybe a simple regex operation could help here?

renovate bot and others added 7 commits July 28, 2022 14:27
* chore(deps): update actions/checkout action to v3 (#1431)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* chore(deps): update dependency ioredis to v5.2.2 (#1450)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Replace cross-undici-fetch with @whatwg-node/fetch

* chore(deps): update dependency vite to v3

* Fix GraphiQL build

* Go

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Arda TANRIKULU <ardatanrikulu@gmail.com>
@n1ru4l n1ru4l self-requested a review July 28, 2022 12:28
Copy link
Collaborator

@n1ru4l n1ru4l left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the best because I fixed it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants