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

feat(response-cache): accept serverContext as the 2nd param in enabled and session #3285

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ardatan
Copy link
Collaborator

@ardatan ardatan commented May 20, 2024

Fixes #3282

Copy link

changeset-bot bot commented May 20, 2024

🦋 Changeset detected

Latest commit: 80a162d

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

This PR includes changesets to release 23 packages
Name Type
graphql-yoga Minor
@graphql-yoga/plugin-response-cache Major
@graphql-yoga/nestjs Major
@graphql-yoga/render-graphiql Major
@graphql-yoga/plugin-apollo-inline-trace Major
@graphql-yoga/plugin-apq Major
@graphql-yoga/plugin-csrf-prevention Major
@graphql-yoga/plugin-defer-stream Major
@graphql-yoga/plugin-disable-introspection Major
@graphql-yoga/plugin-graphql-sse Major
@graphql-yoga/plugin-jwt Major
@graphql-yoga/plugin-persisted-operations Major
@graphql-yoga/plugin-prometheus Major
@graphql-yoga/plugin-sofa Major
apollo-federation-gateway-with-yoga Patch
apollo-subgraph-with-yoga Patch
graphql-lambda Patch
cloudflare-advanced Patch
cloudflare Patch
functions Patch
nextjs-app Patch
hello-world-benchmark Patch
@graphql-yoga/nestjs-federation Patch

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

@ardatan ardatan requested a review from EmrysMyrddin May 20, 2024 12:08
Copy link
Contributor

✅ Benchmark Results

     ✓ no_errors{mode:graphql}
     ✓ expected_result{mode:graphql}
     ✓ no_errors{mode:graphql-jit}
     ✓ expected_result{mode:graphql-jit}
     ✓ no_errors{mode:graphql-response-cache}
     ✓ expected_result{mode:graphql-response-cache}
     ✓ no_errors{mode:graphql-no-parse-validate-cache}
     ✓ expected_result{mode:graphql-no-parse-validate-cache}

     checks.......................................: 100.00% ✓ 407310      ✗ 0     
     data_received................................: 1.6 GB  14 MB/s
     data_sent....................................: 82 MB   685 kB/s
     http_req_blocked.............................: avg=1.46µs   min=962ns    med=1.32µs   max=298.93µs p(90)=1.88µs   p(95)=2.08µs  
     http_req_connecting..........................: avg=2ns      min=0s       med=0s       max=136.41µs p(90)=0s       p(95)=0s      
     http_req_duration............................: avg=380.09µs min=211.42µs med=337.88µs max=24.08ms  p(90)=528.38µs p(95)=547.3µs 
       { expected_response:true }.................: avg=380.09µs min=211.42µs med=337.88µs max=24.08ms  p(90)=528.38µs p(95)=547.3µs 
     ✓ { mode:graphql-jit }.......................: avg=286.98µs min=211.42µs med=265.89µs max=18.06ms  p(90)=296.81µs p(95)=309.59µs
     ✓ { mode:graphql-no-parse-validate-cache }...: avg=554.38µs min=439.64µs med=527.37µs max=9.93ms   p(90)=566.15µs p(95)=598.08µs
     ✓ { mode:graphql-response-cache }............: avg=361µs    min=273.47µs med=342.29µs max=16.35ms  p(90)=374.24µs p(95)=384.89µs
     ✓ { mode:graphql }...........................: avg=376.97µs min=294.29µs med=343.61µs max=24.08ms  p(90)=388.08µs p(95)=438.15µs
     http_req_failed..............................: 0.00%   ✓ 0           ✗ 203655
     http_req_receiving...........................: avg=33µs     min=16.97µs  med=32.98µs  max=861.27µs p(90)=38.49µs  p(95)=40.67µs 
     http_req_sending.............................: avg=8.01µs   min=5.55µs   med=7.19µs   max=305.08µs p(90)=10.64µs  p(95)=11.28µs 
     http_req_tls_handshaking.....................: avg=0s       min=0s       med=0s       max=0s       p(90)=0s       p(95)=0s      
     http_req_waiting.............................: avg=339.06µs min=180.28µs med=297.39µs max=23.87ms  p(90)=487.32µs p(95)=504.08µs
     http_reqs....................................: 203655  1697.078398/s
     iteration_duration...........................: avg=584.46µs min=369.87µs med=538.24µs max=24.88ms  p(90)=735.59µs p(95)=758.19µs
     iterations...................................: 203655  1697.078398/s
     vus..........................................: 1       min=1         max=1   
     vus_max......................................: 2       min=2         max=2   

Copy link
Contributor

💻 Website Preview

The latest changes are available as preview in: https://587c2c44.graphql-yoga.pages.dev

Copy link
Contributor

Apollo Federation Subgraph Compatibility Results

Federation 1 Support Federation 2 Support
_service🟢
@key (single)🟢
@key (multi)🟢
@key (composite)🟢
repeatable @key🟢
@requires🟢
@provides🟢
federated tracing🟢
@link🟢
@shareable🟢
@tag🟢
@override🟢
@inaccessible🟢
@composeDirective🟢
@interfaceObject🟢

Learn more:

Copy link
Contributor

github-actions bot commented May 20, 2024

🚀 Snapshot Release (alpha)

The latest changes of this PR are available as alpha on npm (based on the declared changesets):

Package Version Info
graphql-yoga-cloud-run-guide 3.4.0-alpha-20240520121858-286afc20 npm ↗︎ unpkg ↗︎
@graphql-yoga/apollo-link 3.4.0-alpha-20240520121858-286afc20 npm ↗︎ unpkg ↗︎
@graphql-yoga/urql-exchange 3.4.0-alpha-20240520121858-286afc20 npm ↗︎ unpkg ↗︎
@graphql-yoga/graphiql 4.3.0-alpha-20240520121858-286afc20 npm ↗︎ unpkg ↗︎
graphql-yoga 5.4.0-alpha-20240520121858-286afc20 npm ↗︎ unpkg ↗︎
@graphql-yoga/nestjs 3.4.0-alpha-20240520121858-286afc20 npm ↗︎ unpkg ↗︎
@graphql-yoga/nestjs-federation 3.4.0-alpha-20240520121858-286afc20 npm ↗︎ unpkg ↗︎
@graphql-yoga/plugin-apollo-inline-trace 3.4.0-alpha-20240520121858-286afc20 npm ↗︎ unpkg ↗︎
@graphql-yoga/plugin-apq 3.4.0-alpha-20240520121858-286afc20 npm ↗︎ unpkg ↗︎
@graphql-yoga/plugin-csrf-prevention 3.4.0-alpha-20240520121858-286afc20 npm ↗︎ unpkg ↗︎
@graphql-yoga/plugin-defer-stream 3.4.0-alpha-20240520121858-286afc20 npm ↗︎ unpkg ↗︎
@graphql-yoga/plugin-disable-introspection 2.4.0-alpha-20240520121858-286afc20 npm ↗︎ unpkg ↗︎
@graphql-yoga/plugin-graphql-sse 3.4.0-alpha-20240520121858-286afc20 npm ↗︎ unpkg ↗︎
@graphql-yoga/plugin-jwt 2.4.0-alpha-20240520121858-286afc20 npm ↗︎ unpkg ↗︎
@graphql-yoga/plugin-persisted-operations 3.4.0-alpha-20240520121858-286afc20 npm ↗︎ unpkg ↗︎
@graphql-yoga/plugin-prometheus 5.1.0-alpha-20240520121858-286afc20 npm ↗︎ unpkg ↗︎
@graphql-yoga/plugin-response-cache 3.6.0-alpha-20240520121858-286afc20 npm ↗︎ unpkg ↗︎
@graphql-yoga/plugin-sofa 3.4.0-alpha-20240520121858-286afc20 npm ↗︎ unpkg ↗︎
@graphql-yoga/render-graphiql 5.4.0-alpha-20240520121858-286afc20 npm ↗︎ unpkg ↗︎

Comment on lines +28 to +29
session: (request: Request, serverContext?: TServerContext) => PromiseOrValue<Maybe<string>>;
enabled?: (request: Request, serverContext?: TServerContext) => boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is serverContext optional? Can conditional optional based on the provided TServerContext type instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know, the server context object passed here is TServerContext | undefined;
https://github.com/dotansimha/graphql-yoga/pull/3285/files#diff-2bb04768b60ebe5776d0a80eafa2b96026b7bae21deda57af2e04bb9e5abb4cbR445
It doesn't work otherwise.

Copy link
Collaborator

Choose a reason for hiding this comment

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

if it is | undefined, serverContext: TServerContext should be fine?

Copy link
Collaborator

Choose a reason for hiding this comment

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

because the ? is the same as if it is undefined?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In what scenario is the server context undefined in real-world usage?

params: GraphQLParams;
request: Request;
serverContext?: TServerContext;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

There are no tests for this + unresolved discussion https://github.com/dotansimha/graphql-yoga/pull/3285/files#r1607853806

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.

[useResponseCache] Provide server context to define session Id
3 participants