New Request Pipeline #1795
Merged
New Request Pipeline #1795
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
…al API." This reverts commit 6d6c9ff.
This reverts commit 96af44e.
This reverts commit 408198e.
This reverts commit 4175f1b.
* Allow an optional function to resolve the rootValue Passes the parsed DocumentNode AST to determine the root value, useful when providing a different rootValue for query vs mutation * Add API docs for rootValue
* Provide ability to specify client info in traces Adds the createClientInfo to apollo-engine-reporting, which enables the differentiation of clients based on the request, operation, and variables. This could be extended to include the response. However for the first release. It doesn't quite make sense. * Use extensions and context in createClientInfo * Remove support for clientAddress The frontend will not support it in the near future * create -> generate and make default generator createClientInfo -> generateClientInfo * Clarify default values
The `generateClientInfo` API, used to set client identification attributes within traces, is an experimental API and is subject to removal or change in a future (major) Apollo Server release. Ref: #1631
…tests. The linting will happen in CircleCI. Additionally, we already have post-commit hooks so doing linting on every test run seems to just be generally slowing down development since any linting problems are automatically fixed when I do a commit.
The ability to pass in functions or promises shouldn't affect the type variable.
…pto`. This silences the deprecation messages which have been showing up in recent local development on the new request pipeline work. e.g.: > ``` > [DEP0010] DeprecationWarning: crypto.createCredentials is deprecated. > Use tls.createSecureContext instead. > [DEP0011] DeprecationWarning: > crypto.Credentials is deprecated. Use tls.SecureContext instead. > ```
The actual resolving of the context happens in ApolloServer#graphQLServerOptions, but errors thrown while doing that are currently converted to a throwing function.
…pto`. This silences the deprecation messages which have been showing up in recent local development on the new request pipeline work. e.g.: > ``` > [DEP0010] DeprecationWarning: crypto.createCredentials is deprecated. > Use tls.createSecureContext instead. > [DEP0011] DeprecationWarning: > crypto.Credentials is deprecated. Use tls.SecureContext instead. > ```
While cacheControl can take a boolean when passing it in to ApolloServer as part of a Config, these will be converted to default options before passing them on as options.
* Pass the context along to all the extension methods Addresses #1343 With this change you should now be able to implement an extension like so: ```javascript class MyErrorTrackingExtension extends GraphQLExtension { willSendResponse(o) { const { context, graphqlResponse } = o context.trackErrors(graphqlResponse.errors) return o } } ``` Edit by @evans : fixes #1343 fixes #614 as the request object can be taken from context or from requestDidStart fixes #631 "" * Remove context from extra extension functions The context shouldn't be necessary for format, parse, validation, and execute. Format generally outputs the state of the extension. Parse and validate don't depend on the context. Execution includes the context argument as contextValue * Move change entry under vNext
…eline. Prior to embarking on the request pipeline work, and to facilitate development on the new request pipeline, @martijnwalraven and myself intentionally reverted some commits that had been recently introduced but conflicted with the request pipeline branch that was already in-flight. Rather than dealing with an incredibly difficult merge conflict, it was easier to revert them an re-apply them later. Original commits, reversions, and reintroductions: * 6d6c9ff, 03a894d, b90ccc2 * 96af44e, 68c82e6, 81c4642 * 408198e, 2b470e1, 84bc834 * 4175f1b, 38e7b6a, 261994c As is demonstrated by this short follow-up commit, this was all that was necessary to make it work in the new model - once that model was finished. While we're certain that the new request pipeline and its plugin model will actually better support some of the behavior that required these initial commits in the name of not introducing breaking changes, we feel it's necessary to maintain their support. I will point out, if you are at all affected by the above commits, we may consider deprecating portions of those APIs in the not too distant future, but we hope that the new model will help support those needs even better.
Codecov Report
@@ Coverage Diff @@
## master #1795 +/- ##
=========================================
+ Coverage 73.82% 77.8% +3.98%
=========================================
Files 29 30 +1
Lines 1150 1149 -1
Branches 296 260 -36
=========================================
+ Hits 849 894 +45
+ Misses 290 246 -44
+ Partials 11 9 -2
Continue to review full report at Codecov.
|
4 tasks
This was referenced Oct 18, 2018
Closed
abernix
added a commit
that referenced
this pull request
Oct 23, 2018
This commit follows-up on #1795, which introduced the new request pipeline, and implements the triggers for its new life-cycle hooks within the various integration packages. Previously, the only implementation was within the `apollo-server` package and didn't get triggered for those invoking the `applyMiddleware` method on integrations (e.g. `apollo-server-express`, `...-hapi` and `...-koa`). While in an ideal world, ALL existing `applyMiddleware` functions would be marked as `async` functions and allow us to `await ApolloServer#willStart`, in practice the only `applyMiddleware` which is currently `async` is the one within the Hapi implementation. Therefore, we'll instead kick off the `willStart` lifecycle hook as soon as `applyMiddleware` is started, return as quickly as we have before and then (essentially) yield the completion of Apollo Server's `willStart` prior to serving the first request — thus ensuring the completion of server-startup activities. Similarly, we'll do the same for `createHandler` methods on integrations which don't utilize Node.js server frameworks but don't have `async` handlers (e.g. AWS, Lambda, etc.).
abernix
added a commit
that referenced
this pull request
Oct 26, 2018
This commit follows-up on #1795, which introduced the new request pipeline, and implements the triggers for its new life-cycle hooks within the various integration packages. Previously, the only implementation was within the `apollo-server` package and didn't get triggered for those invoking the `applyMiddleware` method on integrations (e.g. `apollo-server-express`, `...-hapi` and `...-koa`). While in an ideal world, ALL existing `applyMiddleware` functions would be marked as `async` functions and allow us to `await ApolloServer#willStart`, in practice the only `applyMiddleware` which is currently `async` is the one within the Hapi implementation. Therefore, we'll instead kick off the `willStart` lifecycle hook as soon as `applyMiddleware` is started, return as quickly as we have before and then (essentially) yield the completion of Apollo Server's `willStart` prior to serving the first request — thus ensuring the completion of server-startup activities. Similarly, we'll do the same for `createHandler` methods on integrations which don't utilize Node.js server frameworks but don't have `async` handlers (e.g. AWS, Lambda, etc.).
This was referenced Nov 14, 2018
abernix
added a commit
that referenced
this pull request
Nov 21, 2018
This is still WIP, but aims to demonstrate some of the new functionality and flexibility introduced in Apollo Server 2.2's new request pipeline (See #1795 for implementation details).
abernix
added a commit
that referenced
this pull request
Apr 9, 2019
The full-query caching feature implemented in #2437 originally took the additional steps of also changing the `apollo-engine-reporting` module to utilize the new request pipeline available in Apollo Server as of #1795. While that would have been nice, there was still some uncertainty about some of the life-cycle hooks that would be necesssary to make that happen and it wasn't worth blocking the implementation of full-query caching on those stalled decisions. Therefore, the changes to utilize new functionality in the request pipeline, including what would have been a simplification of the way that `apollo-engine-reporting` would have obtained the `operationName` (something that is available via the API of the request-pipeline hooks), were backed out and will land separately in a future PR. The portion regarding `operationName` was inadvertently not backed out and instead was left leveraging a life-cycle hook which was not available to the `graphql-extensions` API: `didResolveOperation`. This means that the code was not setting `operationName` in the way it needed to and therefore `operationName` was always being left undefined (as is sometimes permitted!) with this new `apollo-engine-reporting`. This commit puts the functionality back to that which is required for the `graphql-extensions` implementation, and the commit before this (1ae7def) acts as a regression test (it should pass, as of this commit, and fail before it).
abernix
added a commit
that referenced
this pull request
Apr 10, 2019
The full-query caching feature implemented in #2437 originally took the additional steps of also changing the `apollo-engine-reporting` module to utilize the new request pipeline available in Apollo Server as of #1795. While that would have been nice, there was still some uncertainty about some of the life-cycle hooks that would be necesssary to make that happen and it wasn't worth blocking the implementation of full-query caching on those stalled decisions. Therefore, the changes to utilize new functionality in the request pipeline, including what would have been a simplification of the way that `apollo-engine-reporting` would have obtained the `operationName` (something that is available via the API of the request-pipeline hooks), were backed out and will land separately in a future PR. The portion regarding `operationName` was inadvertently not backed out and instead was left leveraging a life-cycle hook which was not available to the `graphql-extensions` API: `didResolveOperation`. This means that the code was not setting `operationName` in the way it needed to and therefore `operationName` was always being left undefined (as is sometimes permitted!) with this new `apollo-engine-reporting`. This commit puts the functionality back to that which is required for the `graphql-extensions` implementation, and the commit before this (8a43341) acts as a regression test (it should pass, as of this commit, and fail before it).
abernix
added a commit
that referenced
this pull request
Apr 11, 2019
* fix: Set `operationName` via `executionDidStart` and `willResolveField`. The full-query caching feature implemented in #2437 originally took the additional steps of also changing the `apollo-engine-reporting` module to utilize the new request pipeline available in Apollo Server as of #1795. While that would have been nice, there was still some uncertainty about some of the life-cycle hooks that would be necesssary to make that happen and it wasn't worth blocking the implementation of full-query caching on those stalled decisions. Therefore, the changes to utilize new functionality in the request pipeline, including what would have been a simplification of the way that `apollo-engine-reporting` would have obtained the `operationName` (something that is available via the API of the request-pipeline hooks), were backed out and will land separately in a future PR. The portion regarding `operationName` was inadvertently not backed out and instead was left leveraging a life-cycle hook which was not available to the `graphql-extensions` API: `didResolveOperation`. This means that the code was not setting `operationName` in the way it needed to and therefore `operationName` was always being left undefined (as is sometimes permitted!) with this new `apollo-engine-reporting`. This commit puts the functionality back to that which is required for the `graphql-extensions` implementation, and the commit before this (8a43341) acts as a regression test (it should pass, as of this commit, and fail before it). * Add a regression test for `operationName` not being defined. This should fail and then be fixed with an upcoming commit.
This was referenced Jul 2, 2019
Closed
StephenBarlow
pushed a commit
that referenced
this pull request
Aug 23, 2019
This is still WIP, but aims to demonstrate some of the new functionality and flexibility introduced in Apollo Server 2.2's new request pipeline (See #1795 for implementation details).
StephenBarlow
pushed a commit
that referenced
this pull request
Aug 28, 2019
This is still WIP, but aims to demonstrate some of the new functionality and flexibility introduced in Apollo Server 2.2's new request pipeline (See #1795 for implementation details).
StephenBarlow
pushed a commit
that referenced
this pull request
Sep 13, 2019
This is still WIP, but aims to demonstrate some of the new functionality and flexibility introduced in Apollo Server 2.2's new request pipeline (See #1795 for implementation details).
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.
The Apollo Server codebase has grown and its complexity has increased in a number of ways over the past years. Over time, a few diverging paths have emerged in the codebase which have become increasingly difficult to traverse and oft required implementing logic in multiple places rather than in a single place.
This is not only a bad welcoming experience for newcomers, who might inaccurately zero in a particular part of the code only to later find out that it's one of three places they need to make a change, but it's also difficult to scale the codebase and test the multiple approaches as throughly as would be ideal.
Through a lot of hard work by @martijnwalraven (as exhibited in the commit history for this PR), the new request pipeline implemented by this PR brings improved clarity to the Apollo Server codebase and — at least for now, no breaking changes. We hope to further boil down some of the logic in Apollo Server into more manageable pieces which we feel will be easier to support for the Apollo team and the community as well.
Additionally, for some time now, Apollo Server has incorporated an experimental API known as
graphql-extensions. That API originally surfaced to implement the instrumentation for the tracing and performance metrics which are consumed via Apollo Engine (in the past via Apollo Engine Proxy and as of Apollo Server 2.0, viaapollo-engine-reporting.The
graphql-extensionsAPI was never intended to be used for general consumption and we've tried to discouraged its more general use, though we know that enough time has gone by without alternatives that there are likely some uses in the wild that aren't under our wing.We've now had the opportunity to think about and identify other integration points which make sense for Apollo Server, based on feedback and struggles that we've seen the community and our partners struggle with. Therefore, this new request pipeline introduces the notion of
pluginswhich offer a number of life-cycle hooks. We've limited the scope of these hooks for now, so we can carefully consider each need, but we hope that it will be a foundation for further extensibility and flexibility — not to mention decoupling of logic which doesn't necessarily belong in the core.We hope to iterate on these, and they are still subject to change, but we're planning on getting these into the next
alphaof Apollo Server. We're working on getting the documentation in line for this new functionality, but for now, we'll leave it as an exercise to the determined.