Add async server.start() function #4981

Previously, server startup worked like this: - `new ApolloServer` - If no gateway, calculate schema and schema derived data immediately - If gateway, kick off gateway.load from the end of the constructor, and if it async-throws, log an error once and make the server kinda broken forever - At various spots in the framework integration code, call (but don't await) the protected `willStart` function, which is an async function that first waits for the gateway to load the schema if necessary and then runs serverWillStart plugin functions; save the Promise returned by calling this. - At request time in the framework integration code, await that Promise. And also, if there's no schema, fail with an error. Now server startup works like this: - ApolloServer represents its state explicitly with a new ServerState - `new ApolloServer` - If no gateway, initialize all the schema-derived state directly like before (though the state now lives inside ServerState) - If gateway, the constructor DOES NOT KICK OFF `gateway.load()` - You can now call `await server.start()` yourself, which will first await `gateway.load` if necessary, and then await all serverWillStart calls. - If you're using `apollo-server` rather than an integration, `server.listen()` will just transparently do this for you; explicit `start()` is just for integrations! - The integration places that used to call willStart now call `server.ensureStarting()` instead which will kick off server.start in the background if you didn't (and log any errors thrown). - The places that used to await promiseWillStart no longer do so; generally right after that code we end up calling `graphqlServerOptions` - `graphqlServerOptions` now awaits `server.ensureStarted` which will start the server if necessary and throw if it threw. The overall change to user experience: - If you're using `apollo-server`, startup errors will cause `listen` to reject; no code changes are necessary. - If you're using an integration you are encouraged to call `await server.start()` yourself immediately after the constructor, which will let you detect startup errors. - But if you don't do that, the server will call `start` itself eventually. When you try to execute your first GraphQL request, `start` will happen if it hasn't already. Also an integration call like `server.applyMiddleware` will initiate a background `start`. If startup fails, the startup error will be logged on *every* failed graphql request, not just the first time like happened before. - If you have your own ApolloServer subclass that calls the protected `willStart` method, it won't work before that method is gone. Consider whether you can eliminate that call by just calling `start`, or perhaps call `ensureStarting` instead. This is close enough to backwards-compatible to be appropriate for a v2 minor release. We are likely to make `start()` required in Apollo Server 3 (other than for `apollo-server`). Also: - Previously we used the deprecated `ApolloServer.schema` field to determine whether to install ApolloServerPluginInlineTrace, which we want to have active by default for federated schemas only. If you're using a gateway, this field isn't actually set at the time that ensurePluginInstantiation reads it. That's basically OK because we don't want to turn on the plugin automatically in the gateway, but in the interest of avoiding use of the deprecated field, I refactored it so that `ApolloServerPluginInlineTrace` is installed by default (ie, if you don't install your own version or install `ApolloServerPluginInlineTraceDisabled`) without checking the schema, and then (if it's installed automatically) it decides whether or not to be active by checking the schema at `serverWillStart` time. - Similarly, schema reporting now throws in its `serverWillStart` if the schema is federated, instead of in `ensurePluginInstantiation`. (This does mean that if you're not using the new `start()` or `apollo-server`, that failure won't make your app fail as fast as if the `ApolloServer` constructor threw.) - Fix some fastify tests that used a fixed listen port to not do that. - I am doing my best to never accidentally run `prettier` on whole files and instead to very carefully select specific blocks of the file to format them several times per minute. Apparently I screwed up once and ran it once on `packages/apollo-server-core/src/ApolloServer.ts`. The ratio of "prettier changes" to "actual changes" in that file is low enough that I'd rather just leave the changes in this PR rather than spending time carefully reverting them. (It's one of the files I work on the most and being able to keep it prettier-clean will be helpful anyway.) - Replace a hacky workaround for the lack of `start` in the op reg tests! - Replace a use of a `Barrier` class I added recently in tests with the `@josephg/resolvable` npm package, which does basically the same thing. Use that package in new tests and in the core state machine itself. - While running tests I found that some test files hung if run separately due to lack of cleanup. I ended up refactoring the cache tests to: - make who is responsible for calling cache.close more consistent - make the Redis client mocks self-contained mocks of the ioredis API instead of starting with an actual ioredis implementation and mocking out some internals - clean up Jest fake timers when a certain test is done I'm not super certain exactly which of these changes fixed the hangs but it does seem better this way. (Specifically I think the fake timer fix, which I did last, is what actually fixed it, but the other changes made it easier for me to reason about what was going on.) Can factor out into another PR if helpful. Fixes #4921. Fixes apollographql/federation#335. TODO: - [ ] Go through all docs and READMEs that have 'FIXME start' and add calls to start. This involves verifying that you can actually do top-level await in the contexts that matter. (eg if it turns out that you really can't call await before you assign a handler in Lambda, that's interesting and may require some other changes to this PR!) - [ ] Actually document start() in the apollo-server reference - [ ] Document start() in all the integrations references - [ ] CHANGELOG - [ ] consider whether removing the protected willStart function is OK

Don't add it for serverless because we don't do that as of today Need to investigate micro still (is it sync-only like serverless?) and write actual docs.

hooray for vercel/micro#335

Co-authored-by: Stephen Barlow <stephenbarlow@APOLLO-StephenBarlow.attlocal.net> Co-authored-by: Stephen Barlow <stephenbarlow@apollo-stephenbarlow.gateway.sonic.net>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add async server.start() function #4981

Add async server.start() function #4981

Commits on Mar 22, 2021