opentelemetry-js
opentelemetry-js copied to clipboard
refactor(context-async-hooks): fix eslint warnings
Which problem is this PR solving?
Fix the following eslint warnings in the opentelemetry-context-async-hooks package:
/home/runner/work/opentelemetry-js/opentelemetry-js/packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts
72:35 warning Don't use `Function` as a type @typescript-eslint/ban-types
134:60 warning Don't use `Function` as a type @typescript-eslint/ban-types
152:64 warning Don't use `Function` as a type @typescript-eslint/ban-types
176:15 warning Don't use `Function` as a type @typescript-eslint/ban-types
/home/runner/work/opentelemetry-js/opentelemetry-js/packages/opentelemetry-context-async-hooks/src/AsyncHooksContextManager.ts
49:22 warning Forbidden non-null assertion @typescript-eslint/no-non-null-assertion
Short description of the changes
The first set of warnings are more involved, the second one just didn't turn out to be necessary at all and the non-null assertion (the ! postfix operator) can simply be removed.
Explaination:
When using typeof foo === 'function' TypeScript narrows the type of foo to Function, so it may be tempting to use Function in signatures when you want to accept any callable function.
However, this is not quite what Function means. Function as a type in TypeScript has inherited a fair bit of historical baggage and behaves strangely.
For the most part, it only guarentee that it has the Function prototype, so has things like .name, .bind and .call on it, but not much beyond that.
For one thing, it includes things like class constructors which are not callable (not without the new keyword), but TypeScript will still let you call it, but the return value is hardcoded to any. At the same time though, it won't let you assign a type of Function to a signature type (e.g. (...args: any[]) => any)) without an explicit cast.
So generally, Function probably doesn't do what you want, has massive footgun around type safety when called, and should be avoided in favor of a suitable signature type, hence the eslint rule forbidding it.
Notably, depending on the position, this is often one of the few cases where you legitimately have to use any over unknown (for the parameters and/or the return type), or else the variance/ subtyping may not work out the way you want. I think there are possibly pre-exisitng issues regarding this in the files I touched, but in the interest of keeping the PR focused and not leaching changes into public API, I did not address those in this commit.
Ref #5365
Type of change
Please delete options that are not relevant.
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] This change requires a documentation update
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
- [x] TypeScript
- [x] eslint
Checklist:
- [x] Followed the style guidelines of this project
- [ ] Unit tests have been added
- [ ] Documentation has been updated
Codecov Report
:x: Patch coverage is 97.50000% with 1 line in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 94.66%. Comparing base (29d0da5) to head (a056ee9).
:warning: Report is 308 commits behind head on main.
| Files with missing lines | Patch % | Lines |
|---|---|---|
| ...sync-hooks/src/AbstractAsyncHooksContextManager.ts | 97.43% | 1 Missing :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## main #5381 +/- ##
==========================================
+ Coverage 94.64% 94.66% +0.01%
==========================================
Files 318 318
Lines 8033 8042 +9
Branches 1688 1688
==========================================
+ Hits 7603 7613 +10
+ Misses 430 429 -1
| Files with missing lines | Coverage Δ | |
|---|---|---|
| ...ontext-async-hooks/src/AsyncHooksContextManager.ts | 100.00% <100.00%> (ø) |
|
| ...sync-hooks/src/AbstractAsyncHooksContextManager.ts | 98.78% <97.43%> (+1.52%) |
:arrow_up: |
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
- :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.
This PR was closed because it has been stale for 14 days with no activity.