-
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
Make the plugin (and gateway) API almost entirely async #5295
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Docs need to be updated, by which I mean I need to search for every single plugin method name and make sure all examples show them as async. |
Before this PR, some plugin methods were sync, some were async, and some returned ValueOrPromise (ie, could be sync or async). This led to a lack of functionality for the never-async methods, confusing inconsistency, and generally vagueness as to what types things are. It's 2021: making a function async is as simple as adding the word `async` and maybe wrapping a return type in `Promise<>`. This PR simplifies matters by making almost every plugin and gateway method always async. The one exception is `willResolveField`, which is called far more than any other plugin method. We already know that schema instrumentation has unfortunate performance overhead so adding to it by adding more Promises to every field doesn't seem like a good idea for now. We reserve the write to change `willResolveField` to a `PromiseOrValue` sometimes-async method later. (Note that in practice, we usually either `await` or call `Promise.all` on the return values from plugins rather than directly calling `.then`, so if you're not using TypeScript you can probably get away with writing sync methods; and if you are using TypeScript the compiler will quickly show you where you're missing your `async`s.) Specifically, this PR: - Makes the following formerly-`ValueOrPromise` methods into async methods: `serverWillStart`, `serverWillStop`, `didResolveSource`, `didResolveOperation`, `didEncounterErrors`, `responseForOperation`, `willSendResponse` - Makes the following formerly-sync methods into async methods: `requestDidStart`, `renderLandingPage`, `parsingDidStart` and its stop hook, `validationDidStart` and its stop hook, `executionDidStart`, `executionDidEnd` - `executionWillStart` can no longer longer return an end hook as a function; `executionWillEnd` must be returned in an object (which was one of two options before) - Changes the methods on `Dispatcher` so that there's only one "sync" method (for `willResolveField`) and the other methods aren't explicitly named with `Async` - Simplifies the `GraphQLService` interface (used for `@apollo/gateway`) to require the `stop` method to exist, to require `executor` to be async, and to note that we always pass `apollo` to `load`. (All recent versions of `@apollo/gateway` satisfy this.) Also require the `GraphQLExecutor` function type to be async. This also avoids the use of `ValueOrPromise<void>`, a somewhat confusing type that means "either a Promise or a return value we shouldn't look at" though we do still have `Promise<X|void>` types. Note that we allow `options` and `context` functions to still have `ValueOrPromise` semantics. Fixes #4103. Fixes #4999. Incorporates work by @lucasconstantino from #4050 and #4051.
glasser
force-pushed
the
glasser/plugin-async
branch
from
June 10, 2021 05:08
aa59765
to
ec750a4
Compare
This was referenced Jun 10, 2021
StephenBarlow
pushed a commit
that referenced
this pull request
Jun 10, 2021
glasser
added a commit
that referenced
this pull request
Jun 15, 2021
- Convert a test about error handling with `graphql-extensions` to be about plugins. - Remove a test that has been skipped for years which tests the `runQuery` API which no longer exists (it's just simulated in the tests). The relevant current API only takes strings and that's fine. - Remove an "AS3" comment about an issue largely resolved in #5295.
glasser
added a commit
that referenced
this pull request
Jun 15, 2021
- Convert a test about error handling with `graphql-extensions` to be about plugins. - Remove a test that has been skipped for years which tests the `runQuery` API which no longer exists (it's just simulated in the tests). The relevant current API only takes strings and that's fine. - Remove an "AS3" comment about an issue largely resolved in #5295. Fixes #5140.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before this PR, some plugin methods were sync, some were async, and some
returned ValueOrPromise (ie, could be sync or async). This led to a lack
of functionality for the never-async methods, confusing inconsistency,
and generally vagueness as to what types things are.
It's 2021: making a function async is as simple as adding the word
async
and maybe wrapping a return type inPromise<>
. This PRsimplifies matters by making almost every plugin and gateway method
always async.
The one exception is
willResolveField
, which is called far more thanany other plugin method. We already know that schema instrumentation has
unfortunate performance overhead so adding to it by adding more Promises
to every field doesn't seem like a good idea for now. We reserve the
write to change
willResolveField
to aPromiseOrValue
sometimes-asyncmethod later.
(Note that in practice, we usually either
await
or callPromise.all
on the return values from plugins rather than directly calling
.then
,so if you're not using TypeScript you can probably get away with writing
sync methods; and if you are using TypeScript the compiler will quickly
show you where you're missing your
async
s.)Specifically, this PR:
ValueOrPromise
methods into asyncmethods:
serverWillStart
,serverWillStop
,didResolveSource
,didResolveOperation
,didEncounterErrors
,responseForOperation
,willSendResponse
requestDidStart
,renderLandingPage
,parsingDidStart
and its stophook,
validationDidStart
and its stop hook,executionDidStart
,executionDidEnd
executionWillStart
can no longer longer return an end hook as afunction;
executionWillEnd
must be returned in an object (which wasone of two options before)
Dispatcher
so that there's only one "sync"method (for
willResolveField
) and the other methods aren't explicitlynamed with
Async
GraphQLService
interface (used for@apollo/gateway
)to require the
stop
method to exist, to requireexecutor
to beasync, and to note that we always pass
apollo
toload
. (All recentversions of
@apollo/gateway
satisfy this.) Also require theGraphQLExecutor
function type to be async.This also avoids the use of
ValueOrPromise<void>
, a somewhat confusingtype that means "either a Promise or a return value we shouldn't look
at" though we do still have
Promise<X|void>
types.Note that we allow
options
andcontext
functions to still haveValueOrPromise
semantics.Fixes #4103. Fixes #4999. Incorporates work by @lucasconstantino from
#4050 and #4051.