Skip to content
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

fix(generic-pool): remove deps on types package for ts5 compatibility #1637

Merged
merged 2 commits into from
Aug 16, 2023

Conversation

blumamir
Copy link
Member

@blumamir blumamir commented Aug 14, 2023

Which problem is this PR solving?

fix #1632

Replace: #1563 . Since the original PR seems inactive, and the fix is relatively easy, I opened this one.

Short description of the changes

removed the types from the instrumentation public generic functions, so they will not show up in instrumentation.d.ts. Now that the package is not part of the public API, it can safely be moved to dev dependency. This practice is common for instrumentation in the repo, as the type of the instrumentation's init function gives very little value (if any) and causes many problems.

After the change, instrumentation.d.ts which is imported from index.d.ts looks like this:

import { InstrumentationBase, InstrumentationConfig, InstrumentationNodeModuleDefinition } from '@opentelemetry/instrumentation';
export default class Instrumentation extends InstrumentationBase<any> {
    private _isDisabled;
    constructor(config?: InstrumentationConfig);
    init(): InstrumentationNodeModuleDefinition<any>[];
    private _acquirePatcher;
    private _poolWrapper;
    private _acquireWithCallbacksPatcher;
}
//# sourceMappingURL=instrumentation.d.ts.map

The generic-pool is not imported in types files.
More info here

@blumamir blumamir requested a review from a team August 14, 2023 17:50
@github-actions github-actions bot requested a review from rauno56 August 14, 2023 17:50
@codecov
Copy link

codecov bot commented Aug 14, 2023

Codecov Report

Merging #1637 (e310634) into main (b3d30af) will not change coverage.
The diff coverage is 100.00%.

❗ Current head e310634 differs from pull request most recent head 52dcddd. Consider uploading reports for the commit 52dcddd to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1637   +/-   ##
=======================================
  Coverage   91.77%   91.77%           
=======================================
  Files         139      139           
  Lines        7112     7112           
  Branches     1427     1427           
=======================================
  Hits         6527     6527           
  Misses        585      585           
Files Changed Coverage Δ
...nstrumentation-generic-pool/src/instrumentation.ts 40.00% <100.00%> (ø)

@blumamir blumamir requested a review from Flarna August 15, 2023 06:27
@blumamir
Copy link
Member Author

@Flarna I know you have a lot of background in these kinds of issues.
Will appreciate your review on this PR when you have some cycles 🙏

Thank you

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for fixing this 🙂

@blumamir blumamir merged commit 651b4f8 into open-telemetry:main Aug 16, 2023
11 checks passed
@dyladan dyladan mentioned this pull request Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot find type definition file for 'generic-pool'
3 participants