-
Notifications
You must be signed in to change notification settings - Fork 789
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: fix lint errors on opentelemetry-context-async-hooks #2338
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,11 @@ import { ContextManager, Context } from '@opentelemetry/api'; | |
import { EventEmitter } from 'events'; | ||
|
||
type Func<T> = (...args: unknown[]) => T; | ||
type UnknownFunc = Func<unknown>; | ||
type UnknownFuncReturns<T extends (...args: unknown[]) => unknown> = (...args: unknown[]) => ReturnType<T>; | ||
|
||
const isUnknownFunc = (fn: unknown): fn is UnknownFunc => typeof fn === 'function'; | ||
const isGenericFunc = <T>(fn: (...args: never[]) => T): fn is Func<T> => typeof fn === 'function'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i don't see why we should differenciate between both, i think the second typeguard is fine to use everywhere no ? |
||
|
||
/** | ||
* Store a map for each event of all original listeners and their "patched" | ||
|
@@ -62,13 +67,13 @@ export abstract class AbstractAsyncHooksContextManager | |
return this._bindEventEmitter(context, target); | ||
} | ||
|
||
if (typeof target === 'function') { | ||
if (isUnknownFunc(target)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This fixes the lint error technically but doesn't actually fix the problem the lint is trying to point out. This will still return true for constructors so doesn't provide any additional safety. IMO it would be better to use a comment to disable the lint warning as a much more obvious solution. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are some workarounds to check if something is a constructor: https://stackoverflow.com/questions/40922531/how-to-check-if-a-javascript-function-is-a-constructor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A comment ignoring the lint error clearly shows "i know this is dangerous and I accept it." A type which obfuscates the danger can come back to bite you later. If a constructor is passed to bind, there really isn't much we can or should do about it but let it fail. edit: i suppose we could just skip and not bind it... @vmarchaud @obecny what do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As far as I know NodeJs uses internally checks like Therefore I don't think we should add a lot complexity here trying to fix a problem which is on caller side. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with @Flarna here, i don't think we had the issue with any instrumentation so adding this much complexity for a problem that we don't have seems pointless right now. I would prefer to wait for an user having this issue to try to fix it |
||
return this._bindFunction(context, target); | ||
} | ||
return target; | ||
} | ||
|
||
private _bindFunction<T extends Function>(context: Context, target: T): T { | ||
private _bindFunction<T extends UnknownFunc>(context: Context, target: T): T { | ||
const manager = this; | ||
const contextWrapper = function (this: never, ...args: unknown[]) { | ||
return manager.with(context, () => target.apply(this, args)); | ||
|
@@ -104,21 +109,24 @@ export abstract class AbstractAsyncHooksContextManager | |
|
||
// patch methods that add a listener to propagate context | ||
ADD_LISTENER_METHODS.forEach(methodName => { | ||
if (ee[methodName] === undefined) return; | ||
ee[methodName] = this._patchAddListener(ee, ee[methodName], context); | ||
const method = ee[methodName]; | ||
if (isGenericFunc(method)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same issue here, your type guard doesn't actually differentiate between this function and any other type of function. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Answered above. |
||
ee[methodName] = this._patchAddListener(ee, method, context); | ||
} | ||
}); | ||
// patch methods that remove a listener | ||
if (typeof ee.removeListener === 'function') { | ||
ee.removeListener = this._patchRemoveListener(ee, ee.removeListener); | ||
const { removeListener, off, removeAllListeners } = ee | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Until we bump to TS 4.4, we'll need to do it. |
||
if (isGenericFunc(removeListener)) { | ||
ee.removeListener = this._patchRemoveListener(ee, removeListener); | ||
} | ||
if (typeof ee.off === 'function') { | ||
ee.off = this._patchRemoveListener(ee, ee.off); | ||
if (isGenericFunc(off)) { | ||
ee.off = this._patchRemoveListener(ee, off); | ||
} | ||
// patch method that remove all listeners | ||
if (typeof ee.removeAllListeners === 'function') { | ||
if (isGenericFunc(removeAllListeners)) { | ||
ee.removeAllListeners = this._patchRemoveAllListeners( | ||
ee, | ||
ee.removeAllListeners | ||
removeAllListeners | ||
); | ||
} | ||
return ee; | ||
|
@@ -130,7 +138,7 @@ export abstract class AbstractAsyncHooksContextManager | |
* @param ee EventEmitter instance | ||
* @param original reference to the patched method | ||
*/ | ||
private _patchRemoveListener(ee: EventEmitter, original: Function) { | ||
private _patchRemoveListener<T extends UnknownFuncReturns<T>>(ee: EventEmitter, original: T) { | ||
const contextManager = this; | ||
return function (this: never, event: string, listener: Func<void>) { | ||
const events = contextManager._getPatchMap(ee)?.[event]; | ||
|
@@ -148,7 +156,7 @@ export abstract class AbstractAsyncHooksContextManager | |
* @param ee EventEmitter instance | ||
* @param original reference to the patched method | ||
*/ | ||
private _patchRemoveAllListeners(ee: EventEmitter, original: Function) { | ||
private _patchRemoveAllListeners<T extends UnknownFuncReturns<T>>(ee: EventEmitter, original: T) { | ||
const contextManager = this; | ||
return function (this: never, event: string) { | ||
const map = contextManager._getPatchMap(ee); | ||
|
@@ -159,7 +167,7 @@ export abstract class AbstractAsyncHooksContextManager | |
delete map[event]; | ||
} | ||
} | ||
return original.apply(this, arguments); | ||
return original.apply(this, arguments as unknown as unknown[]); | ||
}; | ||
} | ||
|
||
|
@@ -170,9 +178,9 @@ export abstract class AbstractAsyncHooksContextManager | |
* @param original reference to the patched method | ||
* @param [context] context to propagate when calling listeners | ||
*/ | ||
private _patchAddListener( | ||
private _patchAddListener<T extends UnknownFuncReturns<T>>( | ||
ee: EventEmitter, | ||
original: Function, | ||
original: T, | ||
context: Context | ||
) { | ||
const contextManager = this; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this type really save us anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just to avoid writing
Func<unknown>
on lines 24 and 76, and I think it's easier to read.But I could remove this type if you prefer.