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.

Sign up for GitHub

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

Jump to bottom

[Experimental] Added support for graphql-transport-ws #694

Closed
wants to merge 1 commit into from
Closed

[Experimental] Added support for graphql-transport-ws #694

wants to merge 1 commit into from

Conversation

fallenwood
Copy link

Add switchable support for graphql-transport-ws subprotocol defined at https://github.com/enisdenjo/graphql-ws/blob/master/PROTOCOL.md.

I have tested locally with a static html client to verify the subscription flow worked, but I am not sure if there is any other scenario that not supported.

Related to #492

The simple client 
<!DOCTYPE html>
<html>
<head>
  <title>ServerSentEvents</title>
  <script
    type="text/javascript"
    src="//unpkg.com/graphql-ws/umd/graphql-ws.min.js>
  </script>
</head>
<body>
  <div>Subscribe Messages</div>
  <div id="messages"></div>
  <script type="text/javascript">
    const messages = document.querySelector("#messages");
    const client = graphqlWs.createClient({
      url: 'ws://127.0.0.1:60341/graphql',
    });

    const payload = `
subscription MessageAdded {
  messageAdded {
    from { id displayName }
    content
  }
}
`;

    client.subscribe({
      query: payload,
     }, {
      next: (data) => {
        const div = document.createElement("div");
        div.innerHTML = JSON.stringify(data);
        messages.appendChild(div);
      },
      error: err => alert("Error! " + err),
      complete: () => alert("Completed"),
    });

  </script>
</body>
</html>

@sungam3r
Copy link
Member

sungam3r commented Jan 1, 2022

@fallenwood Thanks for PR. Honestly, I do not think this is the right time for such PR. This project requires refactoring, simplification, cleaning everything unnecessary around subscriptions. PR brings additional complication, the need to support two protocols. Also note that all PRs with any backward incompatible changes should be targeted on develop branch.

@boulc
Copy link
Contributor

boulc commented Jan 1, 2022

Hey @sungam3r, is there any issue that could be worked on to refactor / simplify and get the code base ready for such a change?

I started also a similar initiative some weeks ago and took the same approach as @fallenwood work. We could all benefit from supporting a more recent and supported subscription protocol.

@sungam3r
Copy link
Member

sungam3r commented Jan 1, 2022

I am not ready to give certain advice. I only know that all code working with subscriptions requires refactoring (moving code around projects, changing dependencies, abstractions, making new projects, etc.). I was going to do this several times, but retreat. I do not use subscriptions, although I have some idea of how they work.

@Shane32
Copy link
Member

Shane32 commented Jan 1, 2022 •
edited
Loading

@fallenwood @boulc

I've been rewriting subscription support from scratch over the last week. It's probably 50% complete. It will not require a dependency on System.Reactive (see graphql-dotnet/graphql-dotnet#2757 ) or System.Threading.Tasks.Dataflow (progress in branch new_subscription_support) anymore, although you can continue to use them for your own code if desired.

NOTE: I could really use assistance testing the new code - when it's ready - since I don't use subscriptions in my own codebases.

Good news is that the proper members are marked virtual in v5.2 so that you can completely replace the subscription implementation with your own implementation. So what I may do (we will see) is create a separate project with the new code for testing, so you can copy the new project into your own codebase and 'plug it into' your existing code by a simple DI registration. Perhaps I would even release a beta nuget package, but I hope not to do so.

If all is successful, this would all be available for all Server v5.2+ users. This does not cover refactoring the project in general, which badly needs to be done, but is waiting on graphql-dotnet/graphql-dotnet#2760 . The refactoring changes would need to go into the next major version of GraphQL and Server (v5 and v6 respectively).

@Shane32
Copy link
Member

Shane32 commented Jan 1, 2022

@fallenwood I really appreciate this PR, even if it is not ultimately merged, so I can see the changes needed for the new transport. I think we may still consider merging this PR for v5.3 if it does not cause breaking changes to existing clients. I would need to spend time to perform a careful review. (I think for now I will be spending my time to continue my rewriting efforts.) Please leave the PR open for now - thanks!

@Shane32 Shane32 added the enhancement New feature or request label Jan 1, 2022
@fallenwood fallenwood changed the base branch from master to develop January 2, 2022 13:58
@fallenwood
Copy link
Author

Really appreciate your comments, I'll change the target to develop (although it's indeed compatible) and leave this one open.

It's not urgent for me - in my project, some other transport is needed because websockets is not supported, and this PR is a side project for me to understand the subscription workflow :) - so I really looking forward to see the new implementation.

Thanks

@sungam3r
Copy link
Member

sungam3r commented Jan 2, 2022

although it's indeed compatible

See tests/ApiApprovalTests/*.approved.txt changes in this PR. You added new arguments in public ctors and did some other things.

@fallenwood
Copy link
Author

although it's indeed compatible

See tests/ApiApprovalTests/*.approved.txt changes in this PR. You added new arguments in public ctors and did some other things.

Ah, yes, you are right

@Shane32 Shane32 mentioned this pull request Mar 16, 2022
Closed
@Shane32 Shane32 mentioned this pull request Apr 14, 2022
Merged
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fallenwood @sungam3r @boulc @Shane32