-
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
Documentation for new request pipeline life-cycle hooks/p… #2008
Conversation
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).
This doesn't address any feedback in particular, I just woke up and decided to re-write a bunch of it.
@abernix Had a look at the preview, and I'm curious what the main differences are between this and the extensions API? |
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.
Love how thorough these docs are, great job @abernix! 🎉 Before we merge, I'd love to see some more info on why plugins are useful for GraphQL developers and what some concrete use cases are. Right now, everything feels a bit abstract. When we release extension points like this (Apollo Link comes to mind on the client), often contributors will build out our suggested use cases for us, so it's a good opportunity to inspire contributors to build their own plugins.
docs/source/features/plugins.md
Outdated
## Overview | ||
|
||
The default Apollo Server installation is designed for a reliable out-of-the-box experience with as little configuration as possible. In order to provide additional features, Apollo Server supports _plugins_ which can interface with various stages of server operation and each request/response cycle. | ||
|
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.
Add a sentence or two about why plugins are valuable for developers with some potential use cases -- it's unclear right now why someone would want to read this docs page.
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.
Thoughts?
For example, these life-cycle hooks might be used to: | |
* Change the HTTP response code depending on the result of the operation's execution using the `willSendResponse` life-cycle hook. | |
* Implement rate-limiting for certain IP addresses using the `requestDidStart` life-cycle hook. |
docs/source/features/plugins.md
Outdated
|
||
The `plugins` array is an array of plugins. They might be provided as a module (and optionally published to a registry — e.g. npm) or defined in-line within the `ApolloServer` constructor. Plugins should be defined correctly and the requirements of building a plugin are explained in the plugin [definition](#definition) section below. | ||
|
||
> **Note:** If a plugin is provided by a package published to a registry (for example, npm), that package must be installed using `npm install <plugin>` or `yarn add <plugin>` prior to use. In-line plugins or plugins which reside locally do not need to be installed. |
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.
I think we can remove this note, since the vast majority of people reading this know how to install a package from a registry.
docs/source/features/plugins.md
Outdated
plugins: [ | ||
|
||
/* A plugin installed from the npm registry. */ | ||
require('apollo-server-operation-registry')({ /* options */ }), |
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.
Semantically, wouldn't most people define variables at the top of the file when they load their modules? I think it's more readable to define const OperationRegistry = require('apollo-server-operation-registry')
at the top of the file and pass OperationRegistry({ /* options */ })
to the plugin array. We can use the line highlighting feature in the code block to draw their attention to both areas of the file.
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.
Maybe; patterns will vary! But this is a fair suggestion.
Here's what it would look like with your suggestion applied: https://5c5ad797c4ed1300088f2dca--apollo-server-docs.netlify.com/docs/apollo-server/features/plugins.html
I thiiinnk...I'm indifferent. The primary reason I had it the other way was to tightly-couple the three examples together. Also, if you think this looks good in the preview, do you think I should change the ./localPluginModule
import (below) as well?
docs/_config.yml
Outdated
@@ -27,6 +27,7 @@ sidebar_categories: | |||
- features/creating-directives | |||
- features/authentication | |||
- features/testing | |||
- features/plugins |
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.
I feel like we should rearchitect the sidebar -- features seems like it's getting too long. What do you think?
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.
I think that's an accurate statement, but I might hold off until we've done some additional Apollo Server 3 planning. Suggestions welcomed though!
docs/source/features/plugins.md
Outdated
|
||
> **Note:** Currently the `pluginInfo` is undefined, but future additions to the plugin API will enable this functionality. For now, the factory function facilities are in place to use, but `pluginInfo` is simply not available. | ||
|
||
## Events |
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.
I think this section should be higher up the hierarchy chain, possibly even above the definition, since it's core to understanding plugins. Then, we can pull Server lifecycle events
and Request lifecycle events
into their own headings with all of the lifecycle methods as subheadings. Right now, it looks strange in the deploy preview that all of the methods are on the same hierarchy level as Server lifecycle events
and Request lifecycle events
.
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.
I think that's a great suggestion; even though it slightly compromises the hierarchy, since it moves Events away from the event details that were under it, I think I like it better too.
Though it's possible that the oddness is partially because we only show the top few levels of headings on the sidebar.
Let me know what you think on https://5c5add5794fb530008ede1aa--apollo-server-docs.netlify.com/docs/apollo-server/features/plugins.html#Events?
docs/source/features/plugins.md
Outdated
|
||
## Events | ||
|
||
There are two main categories of events: server life-cycle events and request life-cycle events. |
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.
Do we want to drop the dash in between life-cycle? In the React docs, they spell it without the dash. https://reactjs.org/docs/state-and-lifecycle.html
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.
Let's drop it! (It's hard to say; life-cycle
has almost a billion matches on Google and lifecycle
only has 128 million. Hard to know if "lifecycle" is more closely associated with CS, but I digress.)
My vote would be that we do it consistently across all our documentation, and based on this search, that would be lifecycle
. I'll make the change in a follow-up commit.
Co-Authored-By: abernix <[email protected]>
docs/_config.yml
Outdated
@@ -27,6 +27,7 @@ sidebar_categories: | |||
- features/creating-directives | |||
- features/authentication | |||
- features/testing | |||
- features/plugins |
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.
I think that's an accurate statement, but I might hold off until we've done some additional Apollo Server 3 planning. Suggestions welcomed though!
docs/source/features/plugins.md
Outdated
## Overview | ||
|
||
The default Apollo Server installation is designed for a reliable out-of-the-box experience with as little configuration as possible. In order to provide additional features, Apollo Server supports _plugins_ which can interface with various stages of server operation and each request/response cycle. | ||
|
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.
Thoughts?
For example, these life-cycle hooks might be used to: | |
* Change the HTTP response code depending on the result of the operation's execution using the `willSendResponse` life-cycle hook. | |
* Implement rate-limiting for certain IP addresses using the `requestDidStart` life-cycle hook. |
docs/source/features/plugins.md
Outdated
|
||
## Events | ||
|
||
There are two main categories of events: server life-cycle events and request life-cycle events. |
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.
Let's drop it! (It's hard to say; life-cycle
has almost a billion matches on Google and lifecycle
only has 128 million. Hard to know if "lifecycle" is more closely associated with CS, but I digress.)
My vote would be that we do it consistently across all our documentation, and based on this search, that would be lifecycle
. I'll make the change in a follow-up commit.
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 for the feedback, @peggyrayzis! I think I've addressed it. Let me know what you think!
docs/source/features/plugins.md
Outdated
plugins: [ | ||
|
||
/* A plugin installed from the npm registry. */ | ||
require('apollo-server-operation-registry')({ /* options */ }), |
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.
Maybe; patterns will vary! But this is a fair suggestion.
Here's what it would look like with your suggestion applied: https://5c5ad797c4ed1300088f2dca--apollo-server-docs.netlify.com/docs/apollo-server/features/plugins.html
I thiiinnk...I'm indifferent. The primary reason I had it the other way was to tightly-couple the three examples together. Also, if you think this looks good in the preview, do you think I should change the ./localPluginModule
import (below) as well?
docs/source/features/plugins.md
Outdated
|
||
> **Note:** Currently the `pluginInfo` is undefined, but future additions to the plugin API will enable this functionality. For now, the factory function facilities are in place to use, but `pluginInfo` is simply not available. | ||
|
||
## Events |
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.
I think that's a great suggestion; even though it slightly compromises the hierarchy, since it moves Events away from the event details that were under it, I think I like it better too.
Though it's possible that the oddness is partially because we only show the top few levels of headings on the sidebar.
Let me know what you think on https://5c5add5794fb530008ede1aa--apollo-server-docs.netlify.com/docs/apollo-server/features/plugins.html#Events?
docs/source/features/plugins.md
Outdated
|
||
### `willSendResponse` | ||
|
||
> TODO |
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.
Worth leaving myself a comment that this is still TODO
and waiting to be filled out.
docs/source/features/plugins.md
Outdated
|
||
### `executionDidStart` | ||
|
||
> TODO |
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.
TODO
.
(As I mentioned at the bottom of #2714) This adds a new life-cycle event to the new request pipeline API called `didEncounterErrors`, which receives `requestContext`'s **unformatted** `errors`. (The `requestContext` represents an individual request within Apollo Server.) These `errors` give access to potentially different `errors` than those received within `response.errors`, which are sent to the client and available on the `willSendResponse` life-cycle hook, since they are **not** passed through the `formatError` transformation. This can allow plugin implementers to take advantage of the actual errors and properties which may be pruned from the error before transmission to the client. While most other request pipeline life-cycle events provide a guarantee of their arguments, this `didEncounterErrors` will have a little bit less certainty since the errors could have happened in parsing, validation, or execution, and thus different contracts would have been made (e.g. we may not have been able to negotiate a `requestContext.document` if we could not parse the operation). This still needs tests and I suspect I'll have some additional changes prior to this becoming official, but it can ship as-is for now and will live in the Apollo Server 2.6.0 alpha for the time-being. Use with minimal risk, but with the caveat that the final API may change. Also, I'll need to add docs to #2008.
(As I mentioned at the bottom of #2714) This adds a new life-cycle event to the new request pipeline API called `didEncounterErrors`, which receives `requestContext`'s **unformatted** `errors`. (The `requestContext` represents an individual request within Apollo Server.) These `errors` give access to potentially different `errors` than those received within `response.errors`, which are sent to the client and available on the `willSendResponse` life-cycle hook, since they are **not** passed through the `formatError` transformation. This can allow plugin implementers to take advantage of the actual errors and properties which may be pruned from the error before transmission to the client. While most other request pipeline life-cycle events provide a guarantee of their arguments, this `didEncounterErrors` will have a little bit less certainty since the errors could have happened in parsing, validation, or execution, and thus different contracts would have been made (e.g. we may not have been able to negotiate a `requestContext.document` if we could not parse the operation). This still needs tests and I suspect I'll have some additional changes prior to this becoming official, but it can ship as-is for now and will live in the Apollo Server 2.6.0 alpha for the time-being. Use with minimal risk, but with the caveat that the final API may change. Also, I'll need to add docs to #2008.
docs/source/features/plugins.md
Outdated
> TODO | ||
|
||
```typescript | ||
willSendResponse?( |
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.
can this be used to modify the response before returning, such as adding additional metrics?
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.
Yes!
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 for including this. If we wanted to alter the extensions object in the response, I noticed that only Approach 1 worked. This feels a little counter-intuitive for devs used to functional programming where state-in
-> function -> state out
.
// Approach 1: Direct mutation of object
const helloPlugin1 = {
requestDidStart() {
return {
willSendResponse(requestContext) {
requestContext.response.extensions.randomField = 'Hello World!';
},
};
},
};
// Approach 2: clone and return object
const _ = require('lodash');
const helloPlugin2 = {
requestDidStart() {
return {
willSendResponse(requestContext) {
const newReqContext = _.cloneDeep(requestContext);
newReqContext.response.extensions.randomField = 'Hello World!';
return newReqContext;
},
};
},
};
docs/source/features/plugins.md
Outdated
> TODO | ||
|
||
```typescript | ||
didEncounterErrors?( |
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.
Can this be used in place of formatErrors
because it is async and has access to context?
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.
Also, yes!
Hi @abernix thanks for the work on the docs. Using |
One major omission from these docs is an explanation of why functions can be returned from the
|
* WIP on plugins copyedit * Fix a broken internal link * Finished edit minus TODO functions * Recategorize plugins article and fill in TODOs
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.
Great work, thank you very much @abernix! ❤️
My understanding from looking at actual plugin implementations is, one may not only observe and change the contents of requests / responses but one might also be able to interrupt or control the flow of the request pipeline, e.g. by throwing an error or rejecting a promise in appropriate situations – maybe also by delegating execution to a different operation than the one resolved..?
I personally would love to read a little bit about the request lifecycle hooks' return values and suggest to put in something about possible effects respectively. I believe many readers could even much easier realize the potential value of creating their own plugin then... 💡
Besides this suggestion, I really like the explanations / docs you provided. Thanks again! 👍
|
||
The `didResolveOperation` event fires after the `graphql` library successfully | ||
determines the operation to execute from a request's `document` AST. At this stage, | ||
both the `operationName` string and `operation` AST are available. |
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.
I think the aforementioned bits about the return value's "purpose" or possible effects (might only be one additional sentence per event) could just be added here and in the other paragraphs respectively.
|
||
* [`parsingDidStart`](#parsingdidstart) | ||
* [`validationDidStart`](#validationdidstart) | ||
* [`didResolveOperation`](#didresolveoperation) |
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.
responseForOperation
should also be included, shouldn't it? I think it was added later only a while ago, see here.
@nether-cat (and @nwalters512, prior to that) I think these are all great suggestions, but at this point, I think we should just land this documentation so it actually gets discovered. It's been sitting here for too long now! Please feel free to open follow-up PRs (or at least issues to track your suggestions), particularly for the recently added |
@abernix I agree! I remember me kind of having a hard time with this topic, until I did not only find your PR, but also realized that there even was a preview available :D |
This is still WIP, but aims to document some of the new functionality and flexibility introduced in Apollo Server 2.2's new request pipeline (See #1795 for implementation details).
Early feedback appreciated! (The best bet to review this is likely to check the Netlify preview)