-
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
Enables context creation to be async and capture errors with opt-in logging #1024
Changes from all commits
dcac1b0
879a289
97cc12a
2f3eba2
f348355
dc07e92
dfccfa6
2a36f0f
f1c1e79
2f5d243
35420e8
cf48115
b101384
fdd341c
5fa8d88
9522c5a
6be12f1
7a284d5
4225c08
a08594f
fd79cf7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,9 +17,9 @@ import { | |
ExecutionParams, | ||
} from 'subscriptions-transport-ws'; | ||
|
||
import { internalFormatError } from './errors'; | ||
import { formatApolloErrors } from './errors'; | ||
import { GraphQLServerOptions as GraphQLOptions } from './graphqlOptions'; | ||
import { LogFunction } from './runQuery'; | ||
import { LogFunction, LogAction, LogStep } from './logging'; | ||
|
||
import { | ||
Config, | ||
|
@@ -213,7 +213,12 @@ export class ApolloServerBase<Request = RequestInit> { | |
connection.formatResponse = (value: ExecutionResult) => ({ | ||
...value, | ||
errors: | ||
value.errors && value.errors.map(err => internalFormatError(err)), | ||
value.errors && | ||
formatApolloErrors(value.errors, { | ||
formatter: this.requestOptions.formatError, | ||
debug: this.requestOptions.debug, | ||
logFunction: this.requestOptions.logFunction, | ||
}), | ||
}); | ||
let context: Context = this.context ? this.context : { connection }; | ||
|
||
|
@@ -223,8 +228,11 @@ export class ApolloServerBase<Request = RequestInit> { | |
? await this.context({ connection }) | ||
: context; | ||
} catch (e) { | ||
console.error(e); | ||
throw e; | ||
throw formatApolloErrors([e], { | ||
formatter: this.requestOptions.formatError, | ||
debug: this.requestOptions.debug, | ||
logFunction: this.requestOptions.logFunction, | ||
})[0]; | ||
} | ||
|
||
return { ...connection, context }; | ||
|
@@ -267,20 +275,19 @@ export class ApolloServerBase<Request = RequestInit> { | |
if (this.engine || engineInRequestPath) this.engineEnabled = true; | ||
} | ||
|
||
async request(request: Request) { | ||
request(request: Request) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just want to reiterate now that 2.0 is me and you that I find the name of this method to be incomprehensible and I hope we change it before release. Doesn't have to be in this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Absolutely agree! Will be another PR most likely |
||
let context: Context = this.context ? this.context : { request }; | ||
|
||
//Defer context resolution to inside of runQuery | ||
context = | ||
typeof this.context === 'function' | ||
? await this.context({ req: request }) | ||
? () => this.context({ req: request }) | ||
: context; | ||
|
||
return { | ||
schema: this.schema, | ||
tracing: Boolean(this.engineEnabled), | ||
cacheControl: Boolean(this.engineEnabled), | ||
formatError: (e: GraphQLError) => | ||
internalFormatError(e, this.requestOptions.debug), | ||
context, | ||
// allow overrides from options | ||
...this.requestOptions, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
export enum LogAction { | ||
request, | ||
parse, | ||
validation, | ||
execute, | ||
setup, | ||
cleanup, | ||
} | ||
|
||
export enum LogStep { | ||
start, | ||
end, | ||
status, | ||
} | ||
|
||
export interface LogMessage { | ||
action: LogAction; | ||
step: LogStep; | ||
key?: string; | ||
data?: any; | ||
} | ||
|
||
export interface LogFunction { | ||
(message: LogMessage); | ||
} |
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.
What's the motivation here? (I can certainly believe it's a pain to maintain Node 4 compat but I'm curious what the straw that broke the camel's back was.)
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.
https://circleci.com/gh/apollographql/apollo-server/4564?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link
The same code and setup lead to a different error code. Possibly worth investigating, though I think it's more distracting than valuable