-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Restore "batteries-include" ApolloServer
(v4)
#6420
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 4558722:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking great
Apollo Server isn't path-aware anymore. Testing the path in integration tests is really just testing that express is routing correctly.
This reverts commit b3dfd74.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first I was surprised that the zillion tests that show "post to /graphql" don't break but I guess it's because we ignore the path. I think maybe a follow-up PR could replace all the /graphql
with /
in the tests?
const standaloneServerInstance = standaloneServer(server, opts); | ||
await standaloneServerInstance.listen({ port: 0 }); | ||
|
||
return { server, httpServer: standaloneServerInstance['httpServer'] }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might make sense to update the integration test's API to take an URL instead of an http.Server (are there any uses of the httpServer that can't be replaced by an URL? supertest can). That way we could support integrations that somehow don't literally use an http.Server (maybe some serverless things might work that way?) as long as they can serve at an URL. I think this could be a follow-up PR though!
Not quite sure why the sample code is showing that try/catch though (which logs the last line even on error) rather than just
(also the example doesn't work due to having non-base context but no |
Updated example to remove try/catch, but we agreed sync the context is actually fine. Will follow up in a separate PR with those changes! |
Resolved in #6425 |
This PR (re) introduces the "batteries-included" version of
ApolloServer
. This server manages anexpress
instance internally, meaning it is explicitly not configurable. This is intended to be a nice getting started experience, but more complicated use cases should "eject" to theexpressMiddleware
(i.e. serving various paths, installing other middlewares, etc.)The usage is as follows:
standaloneServer
returns anApolloServerStandalone
instance. The intention is toawait
the call tolisten()
on this class. Thelisten()
method accepts the same arguments thathttp.Server
'slisten()
method does (in the options form only).Additionally, this PR introduces a new public function on the
ApolloServer
class,addPlugin
. This function can be used to add plugins toApolloServer
(only in itsinitialized
state). This enables us to use the drain http server plugin with the approach we've taken since theApolloServer
instance (and its plugins) are instantiated before thehttpServer
is, which the drain plugin needs a reference to. This API enables other "standalone server" authors to do the same if they choose.Fixes #6085