Add pluggable executor to request pipeline #2416
Closed
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
martijnwalraven
force-pushed
the
add-pluggable-executor
branch
from
e16779d
to
185dd45
Compare
abernix
approved these changes
Mar 7, 2019
| request.operationName, | ||
| request.variables, | ||
| )) as GraphQLResponse; | ||
| response = await execute(requestContext as WithRequired< |
There was a problem hiding this comment.
TypeScript inquisition: Is it necessary to cast requestContext to having those required properties, or is that something that can be automatically inferred by the positional parameter typing on execute itself?
My believe here would have been that it was the latter and that we could remove this additional portion, but perhaps I mistaken.
There was a problem hiding this comment.
Unfortunately, the properties we set on requestContext won't change the type. So even if we know properties are there, TypeScript won't automatically narrow the type. I'm not sure what the restrictions on flow-dependent type narrowing are, because it does work in some other cases. But here we need to use an explicit type assertion.
This adjusts the request pipeline to format errors before calling `willSendResponse`, and adds a `didEncounterErrors` hook to give the Engine reporting extension access to the raw errors.
martijnwalraven
force-pushed
the
add-pluggable-executor
branch
from
56725d8
to
6645f76
Compare
Merged
abernix
added a commit
that referenced
this pull request
Apr 28, 2019
This is no longer necessary after the changes in @jbaxleyiii's #2574 (and @martijnwalraven's #2416), which changed the error reporting to be captured via the newly introduced `didEncounterErrors`, rather than in `willResolveField` where it used to happen. Yay!
abernix
added a commit
that referenced
this pull request
Apr 30, 2019
…s for reporting. (#2618) * Add errorFilter option * update changelog * Rename errorFilter to filterErrors, return GraphQLError or null * Add back `maskErrorDetails` type to maintain support. ...though it's currently not functional as of this commit. * Introduce additional tests for `filterErrors` and `maskErrorDetails`. Even though `maskErrorDetails` is going away, it seems important to continue to test it along with the new functionality introduced by `filterErrors`. Note: These tests highlight some failures in the current `filterErrors` logic which need to be addressed in follow-up commits. * Finish up new `filterErrors` functionality, sans renaming to a new name. The intention is to rename the `filterErrors` function to `rewriteError` in a follow-up commit, but this maintains the previous name during the re-implementation phase. * Add a test that ensures that the `stack` is not transmitted. The new implementation of `rewriteError` (previously `formatErrors` and prior to that `maskErrorDetails`, do nothing to ensure that the `stack` property (i.e. `Error.prototype.stack`) is regenerated using the new combination of `${err.name}: ${err.message`, suffixed with the rest of `Error.captureStackTrace`. That's okay, but we should at least guard against that and make sure that no future code starts to add the `stack` property, without properly redacting it within `rewriteError` internals. That redaction is _slightly_ less than performant (string manipulation), so it didn't seem worth the effort, since we don't actually send it anywhere. * Rename new `filterErrors` function to a more accurate `rewriteError`. While the new `filterErrors` functionality did allow complete filtering of errors prior to reporting them to Apollo Engine, it also provides the ability to change the error entirely. This more thorough functionality deserves a name which more accurately defines its functionality. The use-defined `rewriteError` function (defined within the `engine` configuration on the `ApolloServer` constructor options) can still be used to "filter" an error from going to Engine by re-writing the error to `null` To accomplish this nullification of an error, an explicit `null` can be returned from the `rewriteError` function rather than the normal practice of returning a `GraphQLError` (in a presumably modified form). * Adjust the line lengths of some comments. Because prettier doesn't do it! * Add CHANGELOG.md for `rewriteError`. * Update deprecated to use rewriteError * Update rewriteError in docs - Deprecate `formatError` in docs in favor of `rewriteError` - lint-fix * Remove now-unnecessary guard which checks for `errors`. This is no longer necessary after the changes in @jbaxleyiii's #2574 (and @martijnwalraven's #2416), which changed the error reporting to be captured via the newly introduced `didEncounterErrors`, rather than in `willResolveField` where it used to happen. Yay! * Remove long-skipped test that was never close to working. I don't think this is providing much value at this point and it's been skipped for over a year. It appears that the test was intended to test `reportErrorFunction`, though the description claims that it's trying to test something else entirely. Regardless, the testing of traces is actually handled elsewhere now, so this is good to remove. * Fix test structure after merge. * Partially Revert "Update rewriteError in docs" This partially reverts commit 21b8b86, which inadvertently changed `formatError`, which is a top-level function which can be used to adjust the response to the client, which is intrinsically different than the new `rewriteError` which modifies the shape of the error in the tracing data which is sent to the Apollo Platform. * Update the API section with more clarity. * Update docs for rewriteError. (#2585) * (docs) Add information on `rewriteError` to the `errors.md`. This adds additional clarity fro the new functionality provided in #1639. * Fix syntax error and use `String.prototype.startsWith` rather `St.pr.match`. Regular expressions are just not as approachable! * Update errors example. The previous example here wasn't a wonderful example since it was suggesting a pattern which is unnecessary when `NODE_ENV=production`, where stack traces are automatically hidden. * Update errors.md * Update errors.md * Update CHANGELOG.md Co-authored-by: GerA <lpgera@gmail.com>
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.
No description provided.