Skip to content
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.

Sign up for GitHub

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

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ff79a44
refactor(context-async-hooks): fix eslint warnings
chancancode Jan 27, 2025
a056ee9
Merge branch 'main' into eslint-warning-context-async-hooks
chancancode Jan 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 67 additions & 41 deletions packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,17 @@

type Func<T> = (...args: unknown[]) => T;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using unknown here and in the signature of abstract with() is probably wrong:

https://www.typescriptlang.org/play/?#code/CYUwxgNghgTiAEAzArgOzAFwJYHtXwx2BwAoBKALnlRADcQYBuAKGcigGcP4AVACyyoA5vADezePA4Yo2MPADuWDHwCqqANaocC1AB4AgvBAAPDCFTBuaLTtQBtALoAaeADFjZi1fgkAdAGwQhxUBmTwALwAfPAASiAYyDCoPACeAA4gem5RUSSIqFRulHEJSSkZWTliEpIERKRkLJIAvqyS0rJY8koqBqiphp7mltxQA06uHqYjPv6BMMGh4dGliclpmdm5+YXuJfHrFVvV4nX1xOTN8G1tzCjo2HhIOKRQVNIwgkKuAEZUqGQAFtfgwSr9XhAQOMapJCJcmsw7vxvn5empNNpdPlXoiUcI0co+P1UjicIigA

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.


// `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>>;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With better type checking after the refactors, TypeScript caught this as Node EventEmitter event types can be symbols (at least according to the types)

}

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

// 'addListener' | 'on' | 'once' | 'prependListener' | 'prependOnceListener'
type AddListenerKeys = (typeof ADD_LISTENER_METHODS)[number];
Copy link
Contributor Author

@chancancode chancancode Jan 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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];

type AddListener = EventEmitter[AddListenerKeys];
type RemoveListener = EventEmitter['removeListener'];
type removeAllListeners = EventEmitter['removeAllListeners'];

export abstract class AbstractAsyncHooksContextManager
implements ContextManager
{
Expand All @@ -60,18 +70,21 @@
*/
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 @@
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;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

/**
Expand All @@ -95,18 +103,17 @@
* @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 {
Copy link
Contributor Author

@chancancode chancancode Jan 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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 @@
* @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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

};
}

Expand All @@ -149,18 +163,25 @@
* @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 @@
*/
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 @@
* 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);

Check warning on line 219 in packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts

View check run for this annotation

Codecov / codecov/patch

packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts#L219 

Added line #L219 was not covered by tests
}
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
2 changes: 1 addition & 1 deletion packages/opentelemetry-context-async-hooks/src/AsyncHooksContextManager.ts
View file
Open in desktop
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
Loading