opentelemetry-js icon indicating copy to clipboard operation
opentelemetry-js copied to clipboard

refactor(context-async-hooks): fix eslint warnings

Open chancancode opened this issue 9 months ago • 2 comments
trafficstars

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

chancancode avatar Jan 27 '25 22:01 chancancode

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.

codecov[bot] avatar Jan 27 '25 22:01 codecov[bot]

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.

github-actions[bot] avatar Apr 14 '25 06:04 github-actions[bot]

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.

github-actions[bot] avatar Sep 15 '25 06:09 github-actions[bot]

This PR was closed because it has been stale for 14 days with no activity.

github-actions[bot] avatar Oct 06 '25 06:10 github-actions[bot]