Skip to content

Commit

Permalink
refactor(context-async-hooks): fix eslint warnings
Browse files Browse the repository at this point in the history
```
/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
  • Loading branch information
chancancode committed Jan 27, 2025
1 parent 199fd8d commit ff79a44
Show file tree
Hide file tree
Showing 2 changed files with 68 additions and 42 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,17 @@ import { EventEmitter } from 'events';

type Func<T> = (...args: unknown[]) => T;

// `any` here is intentional for variance, `unknown` won't work
// eslint-disable-next-line @typescript-eslint/no-explicit-any
type AnyFunc = (this: any, ...args: any[]) => any;

/**
* 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>>;
}

const ADD_LISTENER_METHODS = [
Expand All @@ -36,6 +40,12 @@ const ADD_LISTENER_METHODS = [
'prependOnceListener' as const,
];

// 'addListener' | 'on' | 'once' | 'prependListener' | 'prependOnceListener'
type AddListenerKeys = (typeof ADD_LISTENER_METHODS)[number];
type AddListener = EventEmitter[AddListenerKeys];
type RemoveListener = EventEmitter['removeListener'];
type removeAllListeners = EventEmitter['removeAllListeners'];

export abstract class AbstractAsyncHooksContextManager
implements ContextManager
{
Expand All @@ -60,18 +70,21 @@ export abstract class AbstractAsyncHooksContextManager
*/
bind<T>(context: Context, target: T): T {
if (target instanceof EventEmitter) {
return this._bindEventEmitter(context, target);
return this._bindEventEmitter(context, target) as T;
}

if (typeof target === 'function') {
return this._bindFunction(context, target);
return this._bindFunction(context, target as T & AnyFunc);
}
return target;
}

private _bindFunction<T extends Function>(context: Context, target: T): T {
private _bindFunction<T extends AnyFunc>(context: Context, target: T): T {
const manager = this;
const contextWrapper = function (this: never, ...args: unknown[]) {
const contextWrapper = function (
this: ThisParameterType<T>,
...args: Parameters<T>
): ReturnType<T> {
return manager.with(context, () => target.apply(this, args));
};
Object.defineProperty(contextWrapper, 'length', {
Expand All @@ -80,12 +93,7 @@ export abstract class AbstractAsyncHooksContextManager
writable: false,
value: target.length,
});
/**
* It isn't possible to tell Typescript that contextWrapper is the same as T
* so we forced to cast as any here.
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any
return contextWrapper as any;
return contextWrapper as T;
}

/**
Expand All @@ -95,18 +103,17 @@ export abstract class AbstractAsyncHooksContextManager
* @param context the context we want to bind
* @param ee EventEmitter an instance of EventEmitter to patch
*/
private _bindEventEmitter<T extends EventEmitter>(
context: Context,
ee: T
): T {
private _bindEventEmitter(context: Context, ee: EventEmitter): EventEmitter {
const map = this._getPatchMap(ee);
if (map !== undefined) return ee;
this._createPatchMap(ee);

// 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 original = ee[methodName];
if (original) {
ee[methodName] = this._patchAddListener(ee, original, context);
}
});
// patch methods that remove a listener
if (typeof ee.removeListener === 'function') {
Expand All @@ -131,15 +138,22 @@ export abstract class AbstractAsyncHooksContextManager
* @param ee EventEmitter instance
* @param original reference to the patched method
*/
private _patchRemoveListener(ee: EventEmitter, original: Function) {
private _patchRemoveListener(
ee: EventEmitter,
original: RemoveListener
): RemoveListener {
const contextManager = this;
return function (this: never, event: string, listener: Func<void>) {
return function (
this: ThisParameterType<RemoveListener>,
...args: Parameters<RemoveListener>
): ReturnType<RemoveListener> {
const [event, listener] = args;
const events = contextManager._getPatchMap(ee)?.[event];
if (events === undefined) {
return original.call(this, event, listener);
if (events) {
const patchedListener = events.get(listener);
args[1] = patchedListener || listener;
}
const patchedListener = events.get(listener);
return original.call(this, event, patchedListener || listener);
return original.apply(this, args);
};
}

Expand All @@ -149,18 +163,25 @@ export abstract class AbstractAsyncHooksContextManager
* @param ee EventEmitter instance
* @param original reference to the patched method
*/
private _patchRemoveAllListeners(ee: EventEmitter, original: Function) {
private _patchRemoveAllListeners(
ee: EventEmitter,
original: removeAllListeners
): removeAllListeners {
const contextManager = this;
return function (this: never, event: string) {
const map = contextManager._getPatchMap(ee);
if (map !== undefined) {
if (arguments.length === 0) {
return function (
this: ThisParameterType<removeAllListeners>,
...args: Parameters<removeAllListeners>
): ReturnType<removeAllListeners> {
const [event] = args;
const events = contextManager._getPatchMap(ee);
if (events) {
if (args.length === 0) {
contextManager._createPatchMap(ee);
} else if (map[event] !== undefined) {
delete map[event];
} else if (event && events[event]) {
delete events[event];
}
}
return original.apply(this, arguments);
return original.apply(this, args);
};
}

Expand All @@ -173,11 +194,14 @@ export abstract class AbstractAsyncHooksContextManager
*/
private _patchAddListener(
ee: EventEmitter,
original: Function,
original: AddListener,
context: Context
) {
): AddListener {
const contextManager = this;
return function (this: never, event: string, listener: Func<void>) {
return function (
this: ThisParameterType<AddListener>,
...args: Parameters<AddListener>
): ReturnType<AddListener> {
/**
* This check is required to prevent double-wrapping the listener.
* The implementation for ee.once wraps the listener and calls ee.on.
Expand All @@ -187,27 +211,29 @@ export abstract class AbstractAsyncHooksContextManager
* that detection.
*/
if (contextManager._wrapped) {
return original.call(this, event, listener);
return original.apply(this, args);
}
let map = contextManager._getPatchMap(ee);
if (map === undefined) {
map = contextManager._createPatchMap(ee);
const [event, listener] = args;
let events = contextManager._getPatchMap(ee);
if (events === undefined) {
events = contextManager._createPatchMap(ee);
}
let listeners = map[event];
let listeners = events[event];
if (listeners === undefined) {
listeners = new WeakMap();
map[event] = listeners;
events[event] = listeners;
}
const patchedListener = contextManager.bind(context, listener);
// store a weak reference of the user listener to ours
listeners.set(listener, patchedListener);
args[1] = patchedListener;

/**
* See comment at the start of this function for the explanation of this property.
*/
contextManager._wrapped = true;
try {
return original.call(this, event, patchedListener);
return original.apply(this, args);
} finally {
contextManager._wrapped = false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ export class AsyncHooksContextManager extends AbstractAsyncHooksContextManager {
): ReturnType<F> {
this._enterContext(context);
try {
return fn.call(thisArg!, ...args);
return fn.apply(thisArg, args);
} finally {
this._exitContext();
}
Expand Down

0 comments on commit ff79a44

Please sign in to comment.