Update typing for tracer.trace
#2693
Conversation
| trace<T> (name: string, fn: (span?: Span, fn?: (error?: Error) => any) => T): T; | ||
| trace<T> (name: string, fn: (span: Span) => T): T; | ||
| trace<T> (name: string, fn: (span: Span, done: (error?: Error) => string) => T): T; | ||
| trace<T> (name: string, options: TraceOptions & SpanOptions, fn: (span?: Span, done?: (error?: Error) => string) => T): T; |
There was a problem hiding this comment.
The return type of the done callback is/was listed as any (with the no-options signature) and string (with the other signature), but looking at the code it seems to be a void -- should that be corrected too?
There was a problem hiding this comment.
It's any since the return value can be whatever, and the return value of tracer.trace will be the same as what's returned by fn when provided.
There was a problem hiding this comment.
tracer.trace has a generic parameter <T> which matches the return type of fn; however, i'm referring to the done callback (which was - confusingly - also called fn in the current version of the code, though i have proposed to rename it).
Arrows pointing to the old and new callbacks
There was a problem hiding this comment.
note the return type of fn remains as T (as does the return type of trace)
| @@ -81,7 +81,8 @@ export declare interface Tracer extends opentracing.Tracer { | |||
| * If the `orphanable` option is set to false, the function will not be traced | |||
| * unless there is already an active span or `childOf` option. | |||
| */ | |||
| trace<T> (name: string, fn: (span?: Span, fn?: (error?: Error) => any) => T): T; | |||
| trace<T> (name: string, fn: (span: Span) => T): T; | |||
| trace<T> (name: string, fn: (span: Span, done: (error?: Error) => string) => T): T; | |||
There was a problem hiding this comment.
It's a bit unclear what use case is improved by this change. Can you add a test case in docs/test.ts?
There was a problem hiding this comment.
Interestingly enough, docs/test.ts already covers it here.. and typescript has errors in master
(whereas typescript doesn't complain on my patch-1 branch)
There was a problem hiding this comment.
im using VSCode with a bunch of extensions.. but i think the only relevant one here is TypeScript Hero -- can't see anything else that would contribute
There was a problem hiding this comment.
fwiw docs/test.ts has lots of other typescript errors -- at least according to my environment (TypeScript 4.9.4)
There was a problem hiding this comment.
I see these errors as well in VS Code but not when running yarn type:test 🤔 Do you know what we're doing wrong that these aren't captured in the test? Ideally it would be fixed so that we don't end up with regressions for these cases.
There was a problem hiding this comment.
Probably vscode isn't using the same base tsconfig or typescript version.
There was a problem hiding this comment.
I can confirm that if I switch VSCode's TS to the version defined in docs/package.json it still shows errors (in VSCode)
- open a new VSCode window from
dd-trace-js/docs - run
yarnfrom that directory (ie. the VSCode terminal) - open
test.ts- warnings still appear
There was a problem hiding this comment.
Do you know what we're doing wrong that these aren't captured in the test?
Unfortunately I do not know
|
@janeklb is this a PR you're still interested in getting merged? Looks like there was still some open queries. If not I can close it out for now. |
Yes, I would still like to have this merged. My understanding of the discussion threads was/is that they're open with datadog folks. Is there something I can do to help? |
If options are not specified (and therefore if `options.orphanable !== false`) then `span` and the `done` callback are guaranteed to be set
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2693 +/- ##
==========================================
- Coverage 84.70% 84.55% -0.15%
==========================================
Files 237 235 -2
Lines 10147 9972 -175
Branches 33 33
==========================================
- Hits 8595 8432 -163
+ Misses 1552 1540 -12 ☔ View full report in Codecov by Sentry. |
What does this PR do?
Corrects the typings of
tracer.trace:span(and thedonecallback, if provided) will never be undefined if nooptionsargument is passed.Motivation
If options are not passed to
trace(and therefore ifoptions.orphanable !== false), then it is guaranteed thatspan(anddone, if provided) will not beundefined.