dgs-framework

dgs-framework copied to clipboard

Pull request checklist

• [x] Please read our contributor guide
• [x] Consider creating a discussion on the discussion forum first
• [x] Make sure the PR doesn't introduce backward compatibility issues
• [ ] Make sure to have sufficient test cases

Pull Request type

• [ ] Bugfix
• [x] Feature
• [ ] Refactoring (no functional changes, no api changes)
• [ ] Build related changes
• [ ] Other (please describe):

Changes in this PR

Describe the new behavior from this PR, and why it's needed Issue https://github.com/Netflix/dgs-framework/discussions/684

Alternatives considered

Describe alternative implementation you have considered

Thanks for the PR @amondnet. We will test out the changes. My initial thought is that we need a better name for the module as it can be confusing with the existing websockets implementation that we already support. Having multiple websocket protocols can be very confusing for the user.

@srinivasankavitha

My initial thought is that we need a better name for the module as it can be confusing with the existing websockets implementation that we already support. Having multiple websocket protocols can be very confusing for the user.

Yes, I agree. Actually I'm not sure how to name the module. graphql-dgs-subscriptions-websockets and graphql-dgs-subscriptions-sse support only Subscriptions. GraphQL over WebSockets and GraphQL over SSE support all 3 GraphQL operations:Queries, Mutations, Subscriptions. These protocols aims to be standardised and become a part of GraphQL with the help of the foundation’s GraphQL over HTTP work group.

https://github.com/graphql/graphql-over-http/pull/140 https://github.com/graphql/graphql-over-http/pull/163

I see, I missed the part about having support for Queries and mutations as well with websockets. Given that clarification, I think your existing module name does seem descriptive enough actually.

On Sat, Oct 9, 2021 at 7:48 PM amond @.***> wrote:

@srinivasankavitha https://github.com/srinivasankavitha

My initial thought is that we need a better name for the module as it can be confusing with the existing websockets implementation that we already support. Having multiple websocket protocols can be very confusing for the user.

Yes, I agree. Actually I'm not sure how to name the module. graphql-dgs-subscriptions-websockets and graphql-dgs-subscriptions-sse support only Subscriptions. GraphQL over WebSockets https://github.com/enisdenjo/graphql-ws and GraphQL over SSE support all 3 GraphQL operations:Queries, Mutations, Subscriptions. These protocols aims to be standardised and become a part of GraphQL with the help of the foundation’s GraphQL over HTTP https://github.com/graphql/graphql-over-http work group.

graphql/graphql-over-http#140 https://github.com/graphql/graphql-over-http/pull/140 graphql/graphql-over-http#163 https://github.com/graphql/graphql-over-http/pull/163

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Netflix/dgs-framework/pull/686#issuecomment-939395838, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJ5JPXMFEIKSCTL5CIQ7K6DUGD5INANCNFSM5FS24BHQ .

I still need to review and test this PR, will do so this week.

On Mon, Oct 11, 2021 at 11:05 AM Kavitha Srinivasan @.***> wrote:

I see, I missed the part about having support for Queries and mutations as well with websockets. Given that clarification, I think your existing module name does seem descriptive enough actually.

On Sat, Oct 9, 2021 at 7:48 PM amond @.***> wrote:

@srinivasankavitha https://github.com/srinivasankavitha

My initial thought is that we need a better name for the module as it can be confusing with the existing websockets implementation that we already support. Having multiple websocket protocols can be very confusing for the user.

Yes, I agree. Actually I'm not sure how to name the module. graphql-dgs-subscriptions-websockets and graphql-dgs-subscriptions-sse support only Subscriptions. GraphQL over WebSockets https://github.com/enisdenjo/graphql-ws and GraphQL over SSE support all 3 GraphQL operations:Queries, Mutations, Subscriptions. These protocols aims to be standardised and become a part of GraphQL with the help of the foundation’s GraphQL over HTTP https://github.com/graphql/graphql-over-http work group.

graphql/graphql-over-http#140 https://github.com/graphql/graphql-over-http/pull/140 graphql/graphql-over-http#163 https://github.com/graphql/graphql-over-http/pull/163

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Netflix/dgs-framework/pull/686#issuecomment-939395838, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJ5JPXMFEIKSCTL5CIQ7K6DUGD5INANCNFSM5FS24BHQ .

What set up are you using for the client to test the server? Are you using Apollo client with graphql-ws library?

@amondnet - when you get a chance, could you describe your set up that you used to test this? Do you have an apollo client set up with the graphql-ws library or do you have your own client? If you have any client code that you can share, much appreciated.

@srinivasankavitha This weekend I will share testable code.

@amondnet - thanks for adding the test and that looks good. The PR overall looks good to me. I tried to test this with a real client with the DGS based on the docs here: https://github.com/enisdenjo/graphql-ws#use-the-client but it does not seem to work after enabling the new module. Have you been able to set up a client to test this end to end? Could you share what you have, if so? Thanks! For reference, I am trying to set this up with the example app and ui that is part of the dgs framework repo here: https://github.com/Netflix/dgs-framework/tree/master/graphql-dgs-example-shared

Happy to test my (closed source) application with an Apollo client if you like. Is there any config necessary on the DGS side, or does the client negotiate the subprotocol as part of the handshake or something?

Happy to test my (closed source) application with an Apollo client if you like. Is there any config necessary on the DGS side, or does the client negotiate the subprotocol as part of the handshake or something?

That would be really helpful! Unfortunately, the testing will involve cloning and building the forked repo with the changes and publishing a local snapshot that you can use in your DGS app. You will also need to add the new module implementation(project(":graphql-dgs-transports-websockets")) in your DGS app to get this functionality.

@paulbakker

Can this be used together with the existing implementation? E.g. can both modules be active at the same time? It does look like it, because the new module is registering on /graphql.

Yes, we can use together. Websocket uses http only during handshake. https://developer.mozilla.org/en-US/docs/Web/API/WebSockets_API/Writing_WebSocket_servers#the_websocket_handshake

Both WebMVC and Webflux should be supported

Ok. I will add both.

Does this conflict with "normal" queries since it's on /graphql? (I think this should work, but we need to make sure).

No conflict.

Sorry for the delay. I'm still happy to test this if it will help push it through. Unfortunately it's not currently building because of linter errors.

@michaelboyles - I am now able to test this with Apollo client and have some fixes. Will push those in the next day. I will be doing some more testing and likely get this PR merged after next week. Will post updates here. Apologies again for the delay due to conflicting internal priorities.

@amondnet - could you please give me access to push a branch to your fork? I have some fixes based on testing your PR.

@srinivasankavitha I have invited you as a collaborator.

https://github.com/Netflix/dgs-framework/tree/feat-subscriptions-graphql-ws

Merged this PR with additional fixes, implementation for webflux and some refactoring: https://github.com/Netflix/dgs-framework/pull/1200.

I'll close this one out. Thanks @amondnet for all the contributions for this feature.