feat: GraphQL support #31
Conversation
Pull Request Test Coverage Report for Build 105
💛 - Coveralls |
example/__tests__/app.e2e-spec.ts
Outdated
| Object { | ||
| "data": null, | ||
| "errors": Array [ | ||
| [GraphQLError: Cannot read property 'headers' of undefined], |
There was a problem hiding this comment.
parse Context error
example/__tests__/app.e2e-spec.ts
Outdated
| Object { | ||
| "data": null, | ||
| "errors": Array [ | ||
| [GraphQLError: [object Object]], |
There was a problem hiding this comment.
@nestjs/graphql support apollo-server-errors
if capture @nestjs/common Exceptions (ex. throw service class), needs custom exception filters
There was a problem hiding this comment.
I don't understand exactly what you mean.
|
@mentos1386 Hello, please review it. thank you : ) |
lib/raven.interceptor.ts
Outdated
| exception, | ||
| ); | ||
| default: | ||
| return this.captureGraphQLException( |
There was a problem hiding this comment.
graphql is not include default ContextType (refs).
making the default graphql can be dangerous, so make the default optional if necessary.
| return this.captureGraphQLException( | |
| if (!this.options.withGraphql) return this.captureException(scope, exception); | |
| return this.captureGraphQLException( |
There was a problem hiding this comment.
I wouldn't go with the default, and also try to avoid adding withGraphql option.
Are there any other contexts that are added by other modules, in the same way as graphql?
lib/raven.interceptor.ts
Outdated
| const data = Handlers.parseRequest(<any>{}, gqlHost.getContext(), this.options); | ||
| const data = Handlers.parseRequest( | ||
| <any>{}, | ||
| gqlHost.getContext(), |
There was a problem hiding this comment.
gqlHost.getContext() isn't the req object in nest by default, so it shouldn't be used as is here.
This caused missing request metadata in sentry until now when using nest-raven with graphql.
I suggest
const context = gqlHost.getContext();
const data = Handlers.parseRequest(
<any>{},
context.req || context,
this.options,
);
Since it is common to add a req field to the context while configuring nest-graphql, but it is by no means the default behaviour so the current code is provided as fallback.
example/__tests__/app.e2e-spec.ts
Outdated
| }); | ||
| expect(result.errors).toMatchInlineSnapshot(` | ||
| Array [ | ||
| [GraphQLError: [object Object]], |
There was a problem hiding this comment.
|
@mentos1386 @orzarchi thank you reviews, fixed it :) |
| level?: Sentry.Severity; | ||
| context?: 'Http' | 'Ws' | 'Rpc' | "GraphQL"; | ||
| level?: Severity; | ||
| withGraphQL?: boolean; |
|
@mentos1386 Hello. Work additional as needed. than you :) |
|
@9renpoto can you fix that merge conflict and i'll merge this. I'm still not fully in to adding |
|
Thanks @9renpoto! |
Closes #20
Closes #17
test/test-projectmove toexample