bump to @apollo/server v4 #574
Conversation
| tracer, | ||
| ...params | ||
| }: { tracer: MockTracer } & Omit< | ||
| InitOptions<InstanceContext>, |
There was a problem hiding this comment.
Why was this passed as a generic?
|
|
||
| obj.addSpan = function (span: Span, info: GraphQLResolveInfo): void { | ||
| this._spans.set(buildPath(info.path), span); | ||
| const spans = new Map<string, Span>(); |
There was a problem hiding this comment.
why was this attached to the context object? (closure here means it's no longer exposed)
| server?: Tracer; | ||
| local?: Tracer; | ||
| onFieldResolveFinish?: (error: Error | null, result: any, span: Span) => void; | ||
| onFieldResolve?: ( | ||
| source: any, | ||
| args: { [argName: string]: any }, | ||
| context: SpanContext, | ||
| context: TContext, |
There was a problem hiding this comment.
why did the lifecycle methods need to know about the SpanContext? (supposed to be private?)
| if (!requestIsInstanceContextRequest<InstanceContext>(infos)) { | ||
| console.warn( | ||
| "Context in request passed to apollo-opentracing#requestDidStart is not a SpanContext, aborting tracing" | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
removing this here since it implicitly exists given the line above (enforced with typescript)
had to move it to a lifecycle method below tho
| @@ -101,7 +101,7 @@ There might be certain field resolvers that are not worth the tracing, e.g. when | |||
|
|
|||
| ## Modifying span metadata | |||
|
|
|||
| If you'd like to add custom tags or logs to span you can construct the extension with `onRequestResolve`. The function is called with two arguments: span and infos `onRequestResolve?: (span: Span, info: RequestStart)` | |||
| If you'd like to add custom tags or logs to span you can construct the extension with `onRequestResolve`. The function is called with two arguments: span and infos `onRequestResolve?: (span: Span, info: GraphQLRequestContext)` | |||
| obj.getSpanByPath = function (path: ResponsePath): Span | undefined { | ||
| return this._spans.get(buildPath(isArrayPath(path) ? path.prev : path)); | ||
| }; | ||
|
|
||
| obj.addSpan = function (span: Span, info: GraphQLResolveInfo): void { |
There was a problem hiding this comment.
Is context.getSpanByPath and context.addSpan supposed to be part of the public API provided by this library?
I don't see it referenced in README.md, other than: onFieldResolve(source: any, args: { [argName: string]: any }, context: SpanContext, info: GraphQLResolveInfo): Allow users to add extra information to the span which implies either:
- you're supposed to be able to use these fields specifically when doing
onFieldResolve, or; - this is just a consequence of mutating the context object and we're just being honest
(I've interpreted the latter given think about using symbols to hide these, but lmk!)
|
FWIW if anyone needs this before it's merged, I used this https://gist.github.com/magicmark/8c2e83eaa59cb8671ef7ce71b2b9db2f (result of this diff) and applied it with patch-package |
|
@DanielMSchmidt perhaps just accept and merge this? if there are bugs, community can submit bugfixes. can be released as 4.0.0 and mark as pre-release in github release. maybe also add more maintainers? like @magicmark can probably support this 4.x changes? |
fixes #573
@DanielMSchmidt this ended up being a larger diff that i'd anticipated:
index.tsto do this ^ (so we don't export the symbol)