refactor(context-async-hooks): fix eslint warnings #5381
Conversation
| @@ -19,13 +19,17 @@ import { EventEmitter } from 'events'; | |||
|
|
|||
| type Func<T> = (...args: unknown[]) => T; | |||
There was a problem hiding this comment.
Using unknown here and in the signature of abstract with() is probably wrong:
But I am not touching that in this PR. Fixing it on the with signature, in particular, requires some careful analysis, since it's part of the public API. Everything I changed in this PR, in contrast, are private implementation details.
| /** | ||
| * Store a map for each event of all original listeners and their "patched" | ||
| * version. So when a listener is removed by the user, the corresponding | ||
| * patched function will be also removed. | ||
| */ | ||
| interface PatchMap { | ||
| [name: string]: WeakMap<Func<void>, Func<void>>; | ||
| [name: string | symbol]: WeakMap<Func<void>, Func<void>>; |
There was a problem hiding this comment.
With better type checking after the refactors, TypeScript caught this as Node EventEmitter event types can be symbols (at least according to the types)
Codecov ReportAttention: Patch coverage is
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
|
| @@ -36,6 +40,12 @@ const ADD_LISTENER_METHODS = [ | |||
| 'prependOnceListener' as const, | |||
| ]; | |||
|
|
|||
| // 'addListener' | 'on' | 'once' | 'prependListener' | 'prependOnceListener' | |||
| type AddListenerKeys = (typeof ADD_LISTENER_METHODS)[number]; | |||
There was a problem hiding this comment.
TypeScript gymnastics:
const FOO: [string, number, boolean] = [...];
type TString = (typeof FOO)[0];
type TNumber = (typeof FOO)[1];
type TBoolean = (typeof FOO)[2];
type TStringOrNumberOrBoolean = (typeof FOO)[number];
| */ | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| return contextWrapper as any; | ||
| return contextWrapper as T; |
There was a problem hiding this comment.
We didn't have to cast to any here, since the contextWrapper satisfies the call signature of T (because I typed all the arguments and return type in accordance to the original T).
However, a cast is necessary here (and technically, unsafe) because T can encompass more than just the call signature. For example, if the input function has a custom property on it (fn.foo = "foo fn"), then it will be lost with this wrapper. Probably not an issue in the real world in this case, but TypeScript isn't wrong to flag it.
And, in general, even if we have to do a completely unrelated unsafe cast, casting into unknown and then to the target type would always work, without having to involve any and disabling the lint rule.
| context: Context, | ||
| ee: T | ||
| ): T { | ||
| private _bindEventEmitter(context: Context, ee: EventEmitter): EventEmitter { |
There was a problem hiding this comment.
Accepting <T extends EventEmitter> causes the assignment below to require a cast without really adding much in type safety.
The reason is that if the input is T extends EventEmitter, technically, the signature for the ee[methodName] can be different than EventEmitter[methodName], so that assignment won't work. To make that work requires propagating the generic to everywhere else which adds a lot of noise with little benefit (ultimately it will boil down to an unsafe cast somewhere, involving exactly this issue anyway).
| } | ||
| const patchedListener = events.get(listener); | ||
| return original.call(this, event, patchedListener || listener); | ||
| return original.apply(this, args); |
There was a problem hiding this comment.
I changed all the delegating calls in this file to take ...args and pass them through to the original via apply(), mostly for consistency, but also arguably safer. If Node adds extra optional arguments in the future we will pass them through correctly.
``` /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 ``` 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 open-telemetry#5365
chancancode
force-pushed
the
eslint-warning-context-async-hooks
branch
from
9c3087e to
ff79a44
Compare
Which problem is this PR solving?
Fix the following eslint warnings in the opentelemetry-context-async-hooks package:
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 offootoFunction, so it may be tempting to useFunctionin signatures when you want to accept any callable function.However, this is not quite what
Functionmeans.Functionas 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
Functionprototype, so has things like.name,.bindand.callon it, but not much beyond that.For one thing, it includes things like class constructors which are not callable (not without the
newkeyword), but TypeScript will still let you call it, but the return value is hardcoded toany. At the same time though, it won't let you assign a type ofFunctionto a signature type (e.g.(...args: any[]) => any)) without an explicit cast.So generally,
Functionprobably 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
anyoverunknown(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.
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: