-
Notifications
You must be signed in to change notification settings - Fork 21
Conversation
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.
LGTM, added minor nit comments.
@@ -46,7 +48,7 @@ export class RootSpan extends Span implements coreTypes.RootSpan { | |||
} | |||
} | |||
|
|||
startChildSpan(name?: string, kind?: string): Span { | |||
startChildSpan(name?: string, kind?: SpanKind): Span { |
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.
May be in another PR, we should change startChildSpan
signature to accept object instead of two params (similar to startRootSpan
).
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.
Sure, I can do that. Added it here in this PR.
@@ -43,7 +42,7 @@ | |||
"access": "public" | |||
}, | |||
"devDependencies": { | |||
"@opencensus/core": "^0.0.8", | |||
"@opencensus/core": "0.0.9", |
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.
Here we are using "pin" dependency, but using semver range in opencensus-web-core
package. I think we should be consistent across. WDYT?
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.
Good point - this was just a mistake. Changed it to the ^0.0.9
semver range to match opencensus-web-core
and the other dependencies.
This upgrades
@opencensus/core
to version0.0.9
. The new version has changes to the span model, which required and enabled some light refactorings.The enums can't be directly imported from
@opencensus/core
or else that will create a runtime dependency since TypeScript enums (unlike interfaces and union types) have a real runtime result object. A runtime dependency on@opencensus/core
causes problems with the webpack build since it tries to find Node-specific things like async hooks, etc. This resolves the issue by just duplicating the enums in@opencensus/web-core
.