fixup! core: add wrapMethod/unwrapMethod util

Author: isaacs

packages/core/src/utils/object.ts

@@ -83,8 +83,14 @@ export function markFunctionWrapped(wrapped: WrappedFunction, original: WrappedF
 
 /**
  * Wrap a method on an object by name, only if it is not already wrapped.
+ *
+ * Note: to set the wrapped method as a non-enumerable property, pass
+ * false as the `enumerable` argument. This could be detected, but only
+ * by either walking up the prototype chain, or iterating over all fields
+ * in a `for(in)` loop. Neither are an acceptable performance impact for
+ * the rare case where we might patch a non-enumerable method.
  */
-export function wrapMethod<O extends {}, T extends string & keyof O>(obj: O, field: T, wrapped: WrappedFunction): void {
+export function wrapMethod<O extends {}, T extends string & keyof O>(obj: O, field: T, wrapped: WrappedFunction, enumerable: boolean = true): void {
   const original = obj[field];
   if (typeof original !== 'function') {
     throw new Error(`Cannot wrap method: ${field} is not a function`);
@@ -93,7 +99,12 @@ export function wrapMethod<O extends {}, T extends string & keyof O>(obj: O, fie
     throw new Error(`Attempting to wrap method ${field} multiple times`);
   }
   markFunctionWrapped(wrapped, original);
-  addNonEnumerableProperty(obj, field, wrapped);
+  Object.defineProperty(obj, field, {
+    writable: true,
+    configurable: true,
+    enumerable,
+    value: wrapped,
+  })
 }
 
 /**

packages/core/test/lib/utils/object.test.ts

@@ -459,18 +459,33 @@ describe('markFunctionWrapped', () => {
 
 describe('wrapMethod', () => {
   it('can wrap a method on an object and unwrap it later', () => {
-    const wrapped = () => {};
-    const original = () => {};
-    const obj = { m: original };
-    wrapMethod(obj, 'm', wrapped);
-    expect(obj.m).toBe(wrapped);
-    expect((obj.m as WrappedFunction).__sentry_original__).toBe(original);
+    const wrappedEnumerable = () => {};
+    const originalEnumerable = () => {};
+    const wrappedNotEnumerable = () => {};
+    const originalNotEnumerable = () => {};
+    const obj: Record<string, unknown> = {
+      enumerable: originalEnumerable,
+    };
+    Object.defineProperty(obj, 'notEnumerable', {
+      writable: true,
+      configurable: true,
+      enumerable: false,
+      value: originalNotEnumerable,
+    });
+    wrapMethod(obj, 'notEnumerable', wrappedNotEnumerable, false);
+    wrapMethod(obj, 'enumerable', wrappedEnumerable);
+    // does not change enumerability
+    expect(Object.keys(obj)).toStrictEqual(['enumerable']);
+    expect(obj.notEnumerable).toBe(wrappedNotEnumerable);
+    expect((obj.notEnumerable as WrappedFunction).__sentry_original__).toBe(originalNotEnumerable);
+    expect(obj.enumerable).toBe(wrappedEnumerable);
+    expect((obj.enumerable as WrappedFunction).__sentry_original__).toBe(originalEnumerable);
   });
 
   it('throws if misused', () => {
     const wrapped = () => {};
     const original = () => {};
-    const obj = { m: original };
+    const obj = { get m() { return original } };
     wrapMethod(obj, 'm', wrapped);
     expect(() => {
       //@ts-expect-error verify type checking prevents this mistake