-
Notifications
You must be signed in to change notification settings - Fork 927
chore: export shimmer functions #5865
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5865 +/- ##
=======================================
Coverage 95.03% 95.03%
=======================================
Files 306 306
Lines 7969 7969
Branches 1611 1611
=======================================
Hits 7573 7573
Misses 396 396 🚀 New features to boost your workflow:
|
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.
I do not have a strong opinion on exposing this to the public API.
cc: @open-telemetry/javascript-maintainers
@@ -33,3 +33,4 @@ export { | |||
safeExecuteInTheMiddleAsync, | |||
} from './utils'; | |||
export { SemconvStability, semconvStabilityFromStr } from './semconvStability'; | |||
export * from './shimmer' |
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.
Please do explicit exports and make sure you mark what is a type or not. It's necessary since we enabled isolatedModules
in #5697
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.
I do not have a strong opinion on exposing this to the public API.
I also don't have a strong preference. Though if we do export these, we should also deprecate the re-exports on the instrumentation base classes so that people are not left wondering which one is the "correct" one to use now.
Which problem is this PR solving?
I need to override some functionality in my code. I want to use new wrapper functions instead of relying on the old shimmer, which is incorrectly typed. I added an export so that I can use these new functions in my derived code, which is heavily based on OTel.
Fixes # (issue)
Short description of the changes
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: