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

chore: remove skipLibCheck: true from tsconfigs where it is not necessary #2165

Conversation

pichlermarc
Copy link
Member

skipLibCheck: true is set a few instrumentation libraries, even though they're not necessary anymore. The only remaining instrumentation that needs it is @opentelemetry/instrumentation-lru-memoizer.

Copy link

codecov bot commented Apr 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.46%. Comparing base (dfb2dff) to head (fdf76cd).
Report is 94 commits behind head on main.

❗ Current head fdf76cd differs from pull request most recent head 655ecf3. Consider uploading reports for the commit 655ecf3 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2165      +/-   ##
==========================================
- Coverage   90.97%   90.46%   -0.52%     
==========================================
  Files         146      147       +1     
  Lines        7492     7580      +88     
  Branches     1502     1574      +72     
==========================================
+ Hits         6816     6857      +41     
- Misses        676      723      +47     

see 30 files with indirect coverage changes

@pichlermarc pichlermarc marked this pull request as ready for review April 29, 2024 09:17
@pichlermarc pichlermarc requested a review from a team April 29, 2024 09:17
Copy link
Contributor

@david-luna david-luna left a comment

Choose a reason for hiding this comment

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

even though they're not necessary anymore...

LGTM but could I get more context on this? Thanks!

@pichlermarc
Copy link
Member Author

even though they're not necessary anymore...

LGTM but could I get more context on this? Thanks!

We've seen this cause some confusion over at #1985.

It looks like some instrumentations had set skipLibCheck: true. I assume this to be either an unintentional copy-paste from an existing instrumentation that had this option set, or it was necessary back then due to actually missing types. Either way, nowadays, the instrumentation compile fine without that flag, so I think it's best to revert to the more common strict config that we use.

@blumamir
Copy link
Member

The only remaining instrumentation that needs it is @opentelemetry/instrumentation-lru-memoizer.

curious why this instrumentation still need the setting

@pichlermarc
Copy link
Member Author

The only remaining instrumentation that needs it is @opentelemetry/instrumentation-lru-memoizer.

curious why this instrumentation still need the setting

There's some problem with the lru-cache types. I looked into it briefly but couldn't find any way to resolve the situation. I think this may be a problem with the upstream packages we use.

@pichlermarc pichlermarc merged commit d066854 into open-telemetry:main Apr 30, 2024
15 checks passed
@pichlermarc pichlermarc deleted the chore/remove-skiplibcheck branch August 14, 2024 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants