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
Add support for willResolveField
and corresponding end handler.
#3988
Add support for willResolveField
and corresponding end handler.
#3988
Commits on Apr 15, 2020
-
Use named types for the "DidEnd" hooks.
Some of the "didStart" request life-cycle hooks permit returning a "didEnd" function from them which will be invoked upon completion. This wasn't always immediately clear by looking at the in-line signature, but providing named types should make it marginally easier to recognize this in the typings.
-
Merge branch 'abernix/named-DidEnd-hooks' into abernix/graphql-extens…
…ions-deprecation-staging
-
Add support for
willResolveField
anddidResolveField
.This adds two of the life-cycle hooks which were available in the `graphql-extensions` API but missing from the new request pipeline plugin API. These omissions have stood in the way of our own ability to migrate our Apollo-related extensions (e.g. `apollo-cache-control`, `apollo-engine-reporting` in federated and non-federated forms, `apollo-tracing`) to the new plugin API and our intention to deprecate that API which was never intended to be public (and was certainly never documented!). That's not to say that any of the effort to do those migrations is easy (it will absolutely not be), however, this unblocks those efforts.
-
chore: Convert
schemaHash
fromstring
to a faux-paque type.By default, TypeScript uses structural typing (as opposed to nominal typing) Put another way, if it looks like the type and walks like that type, then TypeScript lets it be a type. That's often okay, but it leaves a lot to be desired since a `string` of one type can just be passed in as `string` for that type and TypeScript won't complain. Flow offers opaque types which solve this, but TypeScript doesn't offer this (yet?). This Faux-paque type can be used to gain nominal-esque typing, which is incredibly beneficial during re-factors! For the `schemaHash`, in particular, this is very much a string representation that serves a very particular purpose. Passing it incorrectly somewhere could be problematic, but we can avoid that (particularly as I embark on some re-factoring with it momentarily), by typing it as a more-opaque type prior to refactoring. Such passing around of strings can be common, for example, in positional parameters of functions: like a function that receives five strings, but a parameter ends up being misaligned with its destination. With structural typing, it's completely possible to miss that, but `SchemaHash` will _always_ be a `SchemaHash` with this fauxpaque-typing. Happy to not land this, but I think it provides some value. Input appreciated!
-
Introduce a plugin test harness to facilitate testing of plugins.
This test harness is meant to avoid the need to do the more heavy execution which the request pipeline itself does within `processGraphQLRequest`. I'm not prepared to make this a public-facing harness just yet, but I have reason to believe that it could be beneficial for external plugin authors to take advantage of something like this - possibly within the context of `apollo-server-plugin-base`. There's perhaps a best-of-both-worlds approach here where the request pipeline could be tested against a more precise plugin API contract, but I'm deferring that work for now.
Commits on Apr 16, 2020
-
chore: Convert
schemaHash
fromstring
to a faux-paque type. (#3989)By default, TypeScript uses structural typing (as opposed to nominal typing) Put another way, if it looks like the type and walks like that type, then TypeScript lets it be a type. That's often okay, but it leaves a lot to be desired since a `string` of one type can just be passed in as `string` for that type and TypeScript won't complain. Flow offers opaque types which solve this, but TypeScript doesn't offer this (yet?). This Faux-paque type can be used to gain nominal-esque typing, which is incredibly beneficial during re-factors! For the `schemaHash`, in particular, this is very much a string representation that serves a very particular purpose. Passing it incorrectly somewhere could be problematic, but we can avoid that (particularly as I embark on some re-factoring with it momentarily), by typing it as a more-opaque type prior to refactoring. Such passing around of strings can be common, for example, in positional parameters of functions: like a function that receives five strings, but a parameter ends up being misaligned with its destination. With structural typing, it's completely possible to miss that, but `SchemaHash` will _always_ be a `SchemaHash` with this fauxpaque-typing. Happy to not land this, but I think it provides some value. Input appreciated!
-
Introduce an internal plugin test harness to facilitate plugin… (#3990)
* Introduce a plugin test harness to facilitate testing of plugins. This test harness is meant to avoid the need to do the more heavy execution which the request pipeline itself does within `processGraphQLRequest`. I'm not prepared to make this a public-facing harness just yet, but I have reason to believe that it could be beneficial for external plugin authors to take advantage of something like this - possibly within the context of `apollo-server-plugin-base`. There's perhaps a best-of-both-worlds approach here where the request pipeline could be tested against a more precise plugin API contract, but I'm deferring that work for now.
-
Apply suggestions from code review
Co-Authored-By: Trevor Scheer <trevor@apollographql.com>
-
Rejigger
ensurePluginInstantiation
to accommodate upcoming changes.Inspired by landing some PRs separately and a merge commit that could have been avoided, but also inspired by the following comment by @trevor-scheer whicih made it clear my organization was just a _bit_ off. Ref: #3991 (comment)
-
Merge remote-tracking branch 'origin/abernix/graphql-extensions-depre…
…cation-staging' into abernix/add-wrf
Commits on Apr 27, 2020
-
-
Relocate schema instrumentation to
utils/schemaInstrumentation.ts
.The `requestPipelineAPI.ts`'s purpose was originally to keep typings by themselves. It was compiled using a separate TypeScript compilation stage to avoid some circular dependencies within the repository itself. However, it still proved to be problematic since it required external packages which depended on the entire `apollo-server-core` just to utilize those types (e.g. plugins!) The work in #2990 offloaded the types to their own package that could be depended on but the assertion in [[1]] correctly notes that introducing new functionality, which is largely incompatible with the original intent of the `requestPipelineAPI` file (even though it is now deprecated) is largely a step backward. Therefore, this moves the functionality to a new file called `schemaInstrumentation`, as suggested in the following comment. [1]: https://github.com/apollographql/apollo-server/pull/3988/files#r414666538
Commits on May 6, 2020
-
Merge remote-tracking branch 'origin/master' into abernix/add-willRes…
…olveField-and-didResolveField
-
-
Reintroduce genericism to
Dispatcher
.As I reached to introduce a dispatcher for an interface that wasn't a `GraphQLRequestListener`, it dawned on me that the dispatcher that we already had was pretty close to being correct for this prior to the changes in the linked PR/commits below ([[Ref 1]][[Ref 2]]). Within those changesets, the addition of `GrahpQLRequestListener` as a constraint to the `Dispatcher`'s `T` type was done as a matter of necessity so TypeScript could be updated to the latest version. Further analysis, with a motivation to boot, led me to believe that TypeScript was correct in its newfound determination (circa ~v3.7.3) that the `apply` method didn't exist on the keys of the interface being invoked. Concretely, I'm pretty sure there was no constraint that was actually ensuring they were functions and some degree of indirection caused TypeScript to give up trying to reconcile that. This brings back a generic property to the `Dispatcher` by using an object with `string`-key'd properties and functions as values as the constraint for the entire `Dispatcher` interface, rather than `GraphqLRequestListener` itself. Ref 1: 1169db6 Ref 2: #3618
-
Correct recently added plugin types to exclude
void
.I recently introduced these in 46211b3 within #3985, with the intention of making these more understandable in auto-completion within VSCode. While the effective types that the methods where these types were used remains the same (since I've just shifted the `void` from the wrong place to the right place), removing `void` from these types is arguably a breaking change. Since that's just in the typings (and would be a compilation error, I think), and this was recently introduced recently, I'm going to adjust this for correctness now.
-
Introduce
BaseContext
andDefaultContext
.We use this notation in so many places to represent the context and the default context that we should stop referring to it as `Record<string, any>`. This doesn't go so far as to track all of those down, but this at least makes the types exist so we can pursuit that as some point. Luckily, with structural typing, the use of this or an in-line type elsewhere should resolve to a match.
-
tests: Add additional plugin API hook tests.
These should have accompanied the initial PR which I opened to introduce the `willResolveField` and "did resolve field" hooks to the new plugin API, but alas, I didn't do that. Better late than never, and these will be refactored (in a somewhat minor way) in an upcoming commit.
-
Introduce a
callTargets
method on theDispatcher
.This decomposition of the functionality that was previously embedded within the `invokeHookAsync` method will facilitate the addition of two new methods in a follow-up commit: `invokeHookSync` and `reverseInvokeHookSync`.
-
Switch
willResolveField
to be nested withinexecutionDidStart
.The original approach I took when introducing the `willResolveField` field-level wrapper to the new plugin API was to put it at the same level as other request life-cycle events, like `validationDidStart`, and `parsingDidStart`. However, that approach didn't pay respect to the structured/tree-shape that the new request pipeline aimed to bring. For example, currently, the more granular life-cycle events that take place within the _request_ (like `parsingDidStart` and `executionDidStart`), are defined by returning them from the `requestDidStart` hook. This creates a tiered structure within a plugin's implemented hooks that allow variables to be scoped appropriately and shared within a phase of the life-cycle (even sub-hooks) without needing to pin things onto the context unnecessarily. Additionally, in TypeScript, it allows us to gradually widen/narrow the scope of the types that are applied to the `requestContext` during various stages. E.g., `operationName` isn't available before the parsing phase and therefore the type of the `requestContext` within the `parsingDidStart` should, ideally, reflect that. This commit makes this real and, I think, embraces the previous spirit of the new plugin API. Further, while the initial approach I took switched from an object attached to the `context` (received by resolvers) to a `Symbol`, this commit further simplifies some of the machinery we aree exposing. Specifically, rather than attaching the entire dispatcher to the `context` (which we use from an onion wrapper that we put the schema's resolvers within) we now only attach a method which will invoke the dispatcher's `willResolveField` hook, rather than the entirety of the dispatcher's instance itself. This should hopefully reduce the category of "odd behavior" which could result if users tampered with that symbol, since it could result in unexpected behavior.
Commits on May 7, 2020
-
tests: Introduce a test which demonstrates
fieldResolver
behavior.This commit relates to a review comment here: #3988 (comment) This test is meant to help reason about the concern brought up during PR review. Specifically, the concern was about losing functionality of passing a custom `fieldResolver` to `processGraphQLRequest`. This test aims to show that passing a `fieldResolver` to `runQuery` (which calls into `processGraphQLRequest` _nearly_ directly) and eventually gets passed directly into `graphql`'s `execute` method (in a non-federated context) will still work as expected after it's been wrapped by `wrapField` (in our schema instrumentation mechanisms) when a `willResolveField` plugin hook is defined.
-
refactor(tests): Better helpers for APQ tests in intgr. testsuite.
To be leveraged soon: #3998
-
feat(plugins): Intro.
didResolveSource
to indicate availability of ……"source". This PR targets #3988 but was prompted by a need identified during review in the following comment: https://github.com/apollographql/apollo-server/pull/3998/files#r414911049 As noted, this is tangible clunkiness which could make the implementation of the `ensurePreflight` invocations unnecessary. That defensiveness was only necessary because the extensions API - which #3988 + #3998 aims to render unnecessary - guaranteed the presence of the "source" (the text of the incoming operation) prior to invoking its `requestDidStart` method (which was actually a bit after the request had started, in reality). Hopefully this proves to be a useful life-cycle for other cases!
Commits on May 8, 2020
-
-
chore(types): Remove
DefaultContext
and just leverageBaseContext
.They were the same structurally anyway. For some reason I thought that it looked better in the definitions within the [[Plugin types]] when I first wrote it, but I'm not caught up on it. [Plugin types]: https://github.com/apollographql/apollo-server/blob/1356f008/packages/apollo-server-plugin-base/src/index.ts#L48
-
Use an object, rather than positional params for
willResolveField
.This should prove to be more ergonomic, and also allow us to attach other useful properties to this object (for either external or internal use) in the future. Also, adds a test to make sure we're passing _something_! Ref: #3998 (comment)
-
Remove unreachable code after
callTargets
decomposition.The `callTargets` method was introduced in 4fa6ddd, but I forgot to remove the code it rendered unnecessary/aimed to replace.
Commits on May 11, 2020
-
Attach user-defined
fieldResolver
to context.Previously, the "wrapped"-ness of the new plugin API's ability to invoke the user-defined `fieldResolver` which is provided in the `GraphQLServerOptions` interface was only able to invoke a user's own `fieldResolve` by leveraging an additional wrapping of fields by `graphql-extensions`'s own `wrapField` which was an inner-layer of the onion-style wrapping. Since it was closest to the resolver, it was able obtain a reference to the `fieldResolver` which lives on an instance of the `GraphQLExtensionStack` and properly invoke the user's `fieldResolver`. This also attaches the `fieldResolver` provided by the user on a `Symbol` that is attached to the user's `context`, so we can invoke it (when defined) within the new plugin API's `wrapField`'s `field.resolve` function. Ref: #3988 (comment)
Commits on May 12, 2020
-
Condense guards in
schemaInstrumentation
.Both through modern ECMAScript and removing superfluous lines. Ref: 6564081240#r39093122
-
types: Improve
Dispatcher
'scallTargets
andinvokeHookAsync
types.This came up as a matter of discussion within the referenced [[Comment]] when reviewing #3988. The return value of `targets.map` was previously `any[]`'d which, we would hope, could be improved upon. I took a shot at improving it and struggled with the `UnwrapPromise`s when coupled with an expected `Promise` return of `invokeHookAsync`. Removing the unwrapping would seem to still get us the correct type (i.e. an array of the return value of the stingly-typed life-cycle hook, e.g. `wilSendResponse` or `didEncounterErrors`) at the invocation sites of `invokeHookAsync` within the request pipeline (e.g. [[Example]]), and seems to rid us of the `any[]` within the `Dispatcher`. Perhaps I'm missing something, but this would seem to work. I'm perhaps missing some unexpected conflict with the `ValueOrPromise` return types from the `GraphQLRequestListener` methods, but they all return `ValueOrPromise<void>` right now any time that `invokeHookAsync` is called. [Comment]: https://github.com/apollographql/apollo-server/pull/3988/files#r421665333 [Example]: https://github.com/apollographql/apollo-server/blob/f905eb10/packages/apollo-server-core/src/requestPipeline.ts#L591-L594
-
comment: Add note about typing question and reference to issue.
Possibly fix-able in AS3. Ref: https://github.com/apollographql/apollo-server/pull/3988/files#r421632153 Ref: #4103
-
Merge remote-tracking branch 'origin/master' into abernix/add-willRes…
…olveField-and-didResolveField