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

graphql-ws integration adjustments and tests (V3) #1609

Merged
merged 5 commits into from
Aug 17, 2022

Conversation

enisdenjo
Copy link
Collaborator

@enisdenjo enisdenjo commented Aug 16, 2022

Closes #1513

  • Fix "should execute mutation" test which is failing because the usePreventMutationViaGET plugin is preventing mutations from running over GETs and the WebSocket upgrade request is a GET.

    I was considering checking the headers and skipping the check if Upgrade: websocket; however, the request in the context is a Node request... @ardatan suggestions? Use normalizeNodeRequest from @whatwg-node/server? Include socket in context and skip check if socket is set (this would require WebSocket type)?

    P.S. the test is not failing on v3-graphql-http branch because I dropped the plugin and left the check to graphql-http.

@changeset-bot
Copy link

changeset-bot bot commented Aug 16, 2022

🦋 Changeset detected

Latest commit: 12fa22c

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

This PR includes changesets to release 31 packages
Name Type
graphql-yoga Patch
@graphql-yoga/common Patch
@graphql-yoga/node Patch
apollo-federation-gateway-with-yoga Patch
apollo-subgraph-with-yoga Patch
example-azure-function Patch
example-cloudflare-advanced Patch
example-cloudflare-modules Patch
example-defer-stream Patch
example-error-handling Patch
example-express Patch
example-fastify-modules Patch
example-fastify Patch
example-file-upload-nextjs-pothos Patch
example-file-upload-nexus Patch
example-file-upload Patch
example-generic-auth Patch
example-graphql-config Patch
example-hackernews Patch
example-hello-world Patch
example-koa Patch
example-live-query Patch
example-nextjs-auth Patch
example-nextjs Patch
example-node-esm Patch
example-redis-pub-sub Patch
example-service-worker Patch
example-subscriptions Patch
example-sveltekit Patch
hello-world-benchmark Patch
graphql-lambda 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

@enisdenjo enisdenjo self-assigned this Aug 16, 2022
@ardatan
Copy link
Collaborator

ardatan commented Aug 16, 2022

@enisdenjo enisdenjo marked this pull request as ready for review August 17, 2022 10:12
@enisdenjo
Copy link
Collaborator Author

enisdenjo commented Aug 17, 2022

Fixed with https://github.com/dotansimha/graphql-yoga/tree/fix-graphql-ws-compat.

Made me think, maybe we should implement WebSockets over Fetch API? There's nothing out there for it yet the Fetch Standard has a section for WebSockets. 🤔

@enisdenjo enisdenjo requested a review from ardatan August 17, 2022 10:14
@ardatan
Copy link
Collaborator

ardatan commented Aug 17, 2022

@enisdenjo You are right. There is only WebSocket API but I am not sure it is enough for server implementations.

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