-
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
Add support for willResolveField
and corresponding end handler.
#3988
Add support for willResolveField
and corresponding end handler.
#3988
Conversation
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.
…ions-deprecation-staging
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.
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!
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.
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.
This looks great! One question and one caveat within.
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.
Co-Authored-By: Trevor Scheer <[email protected]>
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)
…cation-staging' into abernix/add-wrf
I will probably want to review this as part of reviewing #3998. (Probably won't get to either today.) |
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 good, only one substantive concern.
willResolveField
and didResolveField
.willResolveField
and corresponding end handler.
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
bc910a6
to
11e885c
Compare
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.
Thanks very much for the review, @glasser. I've made some preliminary adjustments based on your feedback, but will circle back to put some additional thought on some bits tomorrow.
…olveField-and-didResolveField
This introduces a deprecation warning which will be emitted once per extension which is defined in the `extensions` parameter of an Apollo Server configuration. With the introduction of the last missing integration point (`willResolveField`) via the recently landed #3988, the new [[Plugin API]] should now have all of the abilities (a super-set, in fact) of the prior `graphql-extensions` API. Furthermore, rather than being undocumented, rather untested and largely experimental, the new plugin API is considered stable and well-understood in terms of what it aims to support. Its documentation and API are now considered part of the Apollo Server API and we will do our best to maintain first-class support for it, including the addition of new functionality as deemed necessary. As of this commit, we will now print deprecation warnings one time per server start-up for _each_ legacy extension. Since extensions were often defined as factory functions which were invoked on each request - and expected to return an instance of the extension by calling `new` on it - we limit this deprecation warning to once per start-up by attaching a `Symbol` to the `constructor` and skipping the warning when the `Symbol` is already present. An alternative design might use a `Map` to track the `constructor`s at the module-level within `requestPipeline.ts`, but I believe this should be functionally the same. [Plugin API]: https://www.apollographql.com/docs/apollo-server/integrations/plugins/
This introduces a deprecation warning which will be emitted once per extension which is defined in the `extensions` parameter of an Apollo Server configuration. With the introduction of the last missing integration point (`willResolveField`) via the recently landed #3988, the new [[Plugin API]] should now have all of the abilities (a super-set, in fact) of the prior `graphql-extensions` API. Furthermore, rather than being undocumented, rather untested and largely experimental, the new plugin API is considered stable and well-understood in terms of what it aims to support. Its documentation and API are now considered part of the Apollo Server API and we will do our best to maintain first-class support for it, including the addition of new functionality as deemed necessary. As of this commit, we will now print deprecation warnings one time per server start-up for _each_ legacy extension. Since extensions were often defined as factory functions which were invoked on each request - and expected to return an instance of the extension by calling `new` on it - we limit this deprecation warning to once per start-up by attaching a `Symbol` to the `constructor` and skipping the warning when the `Symbol` is already present. An alternative design might use a `Map` to track the `constructor`s at the module-level within `requestPipeline.ts`, but I believe this should be functionally the same. [Plugin API]: https://www.apollographql.com/docs/apollo-server/integrations/plugins/
This introduces a deprecation warning which will be emitted once per extension which is defined in the `extensions` parameter of an Apollo Server configuration. With the introduction of the last missing integration point (`willResolveField`) via the recently landed #3988, the new [[Plugin API]] should now have all of the abilities (a super-set, in fact) of the prior `graphql-extensions` API. Furthermore, rather than being undocumented, rather untested and largely experimental, the new plugin API is considered stable and well-understood in terms of what it aims to support. Its documentation and API are now considered part of the Apollo Server API and we will do our best to maintain first-class support for it, including the addition of new functionality as deemed necessary. As of this commit, we will now print deprecation warnings one time per server start-up for _each_ legacy extension. Since extensions were often defined as factory functions which were invoked on each request - and expected to return an instance of the extension by calling `new` on it - we limit this deprecation warning to once per start-up by attaching a `Symbol` to the `constructor` and skipping the warning when the `Symbol` is already present. An alternative design might use a `Map` to track the `constructor`s at the module-level within `requestPipeline.ts`, but I believe this should be functionally the same. [Plugin API]: https://www.apollographql.com/docs/apollo-server/integrations/plugins/
The `apollo-server-testing` package uses an internal Apollo Server method called `executeOperation` (introduced in [#1909]) in order to power its `createTestClient` functionality. This is the testing practice which is documented within [Integration testing] in the Apollo Server documentation. However, it failed to introduce the same context-cloning which [takes place in `runHttpQuery`][Ref 1], prior to arriving at the main request pipeline. Since the context was not cloned, and we had made the expectation in [#3988] that it was a unique context on every single request (which it was, in a non-testing context), the Symbol we use to implement `willResolveField` was already present [on the request pipeline][Ref 2] when running a subsequent test via `createTestClient`! This commit introduces the same cloning that takes place in `buildRequestContext` within `runHttpQuery`, and adds tests to ensure the behavior is preserved. [Fixes #4170]: #4170 [#1909]: #1909 [Integration testing]: https://www.apollographql.com/docs/apollo-server/testing/testing/ [Ref 1]: https://git.io/Jfou6 [#3988]: #3988 [Ref 2]: https://git.io/Jfouy
The `apollo-server-testing` package uses an internal Apollo Server method called `executeOperation` (introduced in [#1909]) in order to power its `createTestClient` functionality. This is the testing practice which is documented within [Integration testing] in the Apollo Server documentation. However, it failed to introduce the same context-cloning which [takes place in `runHttpQuery`][Ref 1], prior to arriving at the main request pipeline. Since the context was not cloned, and we had made the expectation in [#3988] that it was a unique context on every single request (which it was, in a non-testing context), the Symbol we use to implement `willResolveField` was already present [on the request pipeline][Ref 2] when running a subsequent test via `createTestClient`! This commit introduces the same cloning that takes place in `buildRequestContext` within `runHttpQuery`, and adds tests to ensure the behavior is preserved. [Fixes #4170]: #4170 [#1909]: #1909 [Integration testing]: https://www.apollographql.com/docs/apollo-server/testing/testing/ [Ref 1]: https://git.io/Jfou6 [#3988]: #3988 [Ref 2]: https://git.io/Jfouy
Apollo-Orig-Commit-AS: apollographql/apollo-server@b2073e8
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 (documentation found here!):willResolveField
and a correspondingdidResolveField
. These are introduced in a similar way to how they were in thegraphql-extensions
API (where thedidResolveField
callback is returned fromwillResolveField
). A notable difference is that the technique used to reach the "plugin stack" is through a non-enumerable property, keyed with aSymbol
, on the context received by the resolver, rather than the technique employed by thegraphql-extensions
approach.These omissions from the new plugin API 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 officially intended to be public and never was 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 and can be reviewed (and changed) separately.
If there are questions about the implementation chosen here, everything is strongly modeled after the
graphql-extensions
approach for these same life-cycle hooks. That doesn't meant this is the perfect approach though! We can presumably iterate on this without affecting the plugins which implement this, or at least with somewhat minimal changes to their approach in their integration with Apollo Server itself.Example
The object received by the new
willResolveField
hook is an object representation of what will be received by the resolver itself's positional parameters and is declared in the types for the new hook, as seen here, asGraphQLFieldResolverParams
:apollo-server/packages/apollo-server-types/src/index.ts
Lines 161 to 170 in 52418a4
To get further clarity on that, the type for
GraphQLFieldResolverParams
:apollo-server/packages/apollo-server-types/src/index.ts
Lines 161 to 170 in 52418a4
Put another way, rather than receiving the positional parameters
(source, args, context, info)
, it receives({ source, args, context, info })
which allows easier destructuring and discarding of undesired properties, as would be normal with, e.g.,(_source, _args, _context, info)
, just to get toinfo
).And finally, the example:
TODO
- [ ] DocumentationSee #4104