Import OpenTelemetry types rather than duplicate #659
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Use Typescript's `import type` and `export type` to include the OpenTelemetry types into our package without having to copy the code into our project directly. This was suggested by @unflxw in PR #651 (comment) I'm not sure if this is more or less fragile. If the OpenTelemetry package changes the interface we'd be shipping an older version that doesn't work with older versions anymore. The package from which we import is a dev dependency only, and used during the compile step, and won't ship as a dependency in the final release. I got an issue when using the OpenTelemetry SpanProcessor type for the `onEnd` function. It doesn't have a second argument. The `onStart` also needed another type, from `SpanContext` to `Context`.
|
@unflxw is this what you had in mind? |
|
@tombruijn Yes, this is what I had in mind :) I hoped we could remove the file in If possible, I'd wait for @jeffkreeftmeijer's input on this as well? There may be something I'm missing that makes the previous approach preferable. |
|
Ok, asking for a review from Jeff as well. |
luismiramirez
approved these changes
May 11, 2022
jeffkreeftmeijer
approved these changes
May 16, 2022
tombruijn
added a commit
that referenced
this pull request
Jun 8, 2022
In PR #659 we tried to import the OpenTelemetry types at compile time and include them in our own package, but that's not how TypeScript's `import type` turns out to work. This breaks the app for non-OpenTelemetry users as reported in #684. The easiest fix we can ship right now is to add the package as a runtime dependency rather than only a development dependency. Ideally though, we do not ship additional dependencies for thing not all our users use, but we can figure that out after this immediate fix. I had missed this in PR #659, but we've also already included the `@opentelemetry/api` as a dependency since PR #651. Already doing the same this change does for the `@opentelemetry/sdk-trace-base` package.
tombruijn
added a commit
that referenced
this pull request
Jun 8, 2022
In PR #659 we tried to import the OpenTelemetry types at compile time and include them in our own package, but that's not how TypeScript's `import type` turns out to work. This breaks the app for non-OpenTelemetry users as reported in #684. The easiest fix we can ship right now is to add the package as a runtime dependency rather than only a development dependency. Ideally though, we do not ship additional dependencies for thing not all our users use, but we can figure that out after this immediate fix. I had missed this in PR #659, but we've also already included the `@opentelemetry/api` as a dependency since PR #651. Already doing the same this change does for the `@opentelemetry/sdk-trace-base` package.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Use Typescript's
import typeandexport typeto include theOpenTelemetry types into our package without having to copy the code
into our project directly.
This was suggested by @unflxw in PR #651 (comment)
I'm not sure if this is more or less fragile. If the OpenTelemetry
package changes the interface we'd be shipping an older version that
doesn't work with older versions anymore.
The package from which we import is a dev dependency only, and used
during the compile step, and won't ship as a dependency in the final
release.
I got an issue when using the OpenTelemetry SpanProcessor type for the
onEndfunction. It doesn't have a second argument.The
onStartalso needed another type, fromSpanContexttoContext.