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

fix: options and observeHeadersSent flag #7669

Closed
wants to merge 1 commit into from
Closed

fix: options and observeHeadersSent flag #7669

wants to merge 1 commit into from

Conversation

tomcatmwi
Copy link

The problem:
Currently, ExpressMiddleware will attempt to end the Express request after executing a GraphQL request by calling res.end(). If the request was already closed, or headers were sent earlier, an exception is thrown.

Why is this a problem?
Sometimes a GraphQL request may trigger a long server-side process, and we want to respond in some other way than a simple HTTP response. In the project I'm working on, some requests are received as a GraphQL request, but the response is sent as a Server Sent Event, sometimes up to a minute later.

In such a case, the GraphQL request should be responded immediately with just a HTTP code 200, or a simple response. However, if we do that, ExpressMiddleware will attempt to send another response when the process ends. This is undesirable.

What is the suggestion?
The simplest would be to simply skip sending a response if res.headersSent is true. However, this may also introduce unwanted side effects.

My suggestion introduces an options object alongside context. It currently has only one value, observeHeadersSent. If this flag is true, the middleware will not try to send a HTTP response if headers were already sent.

The options object can be extended later with other options, as needed.

@apollo-cla
Copy link

@tomcatmwi: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@netlify
Copy link

netlify bot commented Jul 29, 2023

👷 Deploy request for apollo-server-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 6cdbf0b

@trevor-scheer
Copy link
Member

@tomcatmwi what's the use case for the SSE in this context? Are you trying to implement subscriptions?

Depending on your needs, you might find the approach I took in this PR helpful.

I don't think that adding fields to the default context object is the right approach for this integration, but if you feel that's necessary for your use case you can absolutely write your own wrapper to accommodate your needs. It's pretty straightforward to integrate with AS4!

@trevor-scheer
Copy link
Member

I'm going to close this PR since I'm fairly certain this is not the direction we want, but happy to continue discussion here and possibly entertain a different, agreed-upon approach if we can think of one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants