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

SSE Transport must be inserted before POST, so add convenience for using the DefaultServer #3278

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

StevenACoffman
Copy link
Collaborator

@StevenACoffman StevenACoffman commented Sep 15, 2024

Replacement for #3153 to allow upgrade from POST request to SSE transport response

In order to enable the experimental Server-Sent Events (SSE) transport, you need to have the precedence before the POST transport, but many folks use the DefaultServer which only allows Adding (appending) transports.

This adds convenience methods to the server to both prepend (which is NOT appropriate for SSE) and a special one for inserting right before POST (which is ideal for SSE).

Signed-off-by: Steve Coffman steve@khanacademy.org

…ing the DefaultServer

Signed-off-by: Steve Coffman <steve@khanacademy.org>
@coveralls
Copy link

Coverage Status

coverage: 56.585% (-0.07%) from 56.657%
when pulling 9f105d1 on sse_transport
into 1272aae on master.

@StevenACoffman StevenACoffman added the SSE Server-Sent Events label Sep 15, 2024
@StevenACoffman
Copy link
Collaborator Author

StevenACoffman commented Sep 15, 2024

@o8x @jmic @UnAfraid People who were using the NewDefaultServer were getting tripped up by an inability to add SSE support since it needs to be inserted in the middle of the transport precedence.

Currently, the server just picks the first available transport, and the NewDefaultServer orders them:

  • Websocket
  • Options
  • GET
  • POST
  • MultipartForm

I did not just enable SSE for NewDefaultServer because when used over HTTP/1.1 (not HTTP/2), SSE suffers from a severe limitation to the maximum number of open connections of 6 per browser. See here

We might need to make our transport selection more sophisticated to enable default out-of-the-box support for SSE on HTTP/2 (or HTTP/3).

@UnAfraid
Copy link
Contributor

UnAfraid commented Sep 15, 2024

@StevenACoffman I got bit by NewDefaultServer before, because it initializes Websocket transport and adding new one (To configure it) doesn't have effect, we could also deprecate it in favor of 'New'.
Alternatively we could add 'Priority' method on the transport and sort on add to avoid the POST before SSE.
Also on add if the transport already exists we could just replace existing one with the provided one

@phughes-scwx
Copy link
Contributor

I'm just a rando, but imo NewDefaultServer is a hindrance to people who are using GQLGen for anything beyond toy projects. It should be deprecated and gqlgen init should add the above lines directly to the server code. Even then, maybe only add Options, Get and Post transports, and update the docs / quick start guide to point out that all additional transports have to be added by hand: so for uploads the first step is "add the multipart form" transport, etc etc.

@StevenACoffman
Copy link
Collaborator Author

Now that #3404 is merged and NewDefaultServer is deprecated, there's less need for this, but it might still be helpful.

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

Successfully merging this pull request may close these issues.

4 participants