From c0210a9f4c77cff43b6c228e27b65e2061106bce Mon Sep 17 00:00:00 2001 From: s1gr1d <32902192+s1gr1d@users.noreply.github.com> Date: Wed, 11 Feb 2026 17:02:55 +0100 Subject: [PATCH 1/7] handleCallbackErrors return type fix --- .../thenable-with-extra-methods/subject.js | 89 +++++++++++++ .../thenable-with-extra-methods/test.ts | 69 ++++++++++ .../core/src/utils/handleCallbackErrors.ts | 126 ++++++++++++++++-- .../utils/handleCallbackErrors-proxy.test.ts | 82 ++++++++++++ 4 files changed, 353 insertions(+), 13 deletions(-) create mode 100644 dev-packages/browser-integration-tests/suites/public-api/startSpan/thenable-with-extra-methods/subject.js create mode 100644 dev-packages/browser-integration-tests/suites/public-api/startSpan/thenable-with-extra-methods/test.ts create mode 100644 packages/core/test/lib/utils/handleCallbackErrors-proxy.test.ts diff --git a/dev-packages/browser-integration-tests/suites/public-api/startSpan/thenable-with-extra-methods/subject.js b/dev-packages/browser-integration-tests/suites/public-api/startSpan/thenable-with-extra-methods/subject.js new file mode 100644 index 000000000000..f09573466ce4 --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/public-api/startSpan/thenable-with-extra-methods/subject.js @@ -0,0 +1,89 @@ +/** + * Test that verifies thenable objects with extra methods (like jQuery's jqXHR) + * preserve those methods when returned from Sentry.startSpan(). + * + * This tests the Proxy fix for the GitHub issue where: + * const jqXHR = Sentry.startSpan({ name: "test" }, () => $.ajax(...)); + * jqXHR.abort(); // Should work! + */ + +// Mock a jqXHR-like object (simulates jQuery.ajax() return value) +function createMockJqXHR() { + let resolvePromise; + const promise = new Promise(resolve => { + resolvePromise = resolve; + }); + + console.log(''); + + // Create an object that has both Promise methods and XHR methods + const mockJqXHR = { + then: promise.then.bind(promise), + catch: promise.catch.bind(promise), + finally: promise.finally.bind(promise), + abort: function () { + window.jqXHRAbortCalled = true; + window.jqXHRAbortResult = 'abort-successful'; + return 'abort-successful'; + }, + // XHR-like properties + status: 0, + readyState: 1, + responseText: '', + }; + + // Resolve after a short delay + //setTimeout(() => resolvePromise({ data: 'test response' }), 50); + + return mockJqXHR; +} + +async function runTest() { + window.jqXHRAbortCalled = false; + window.jqXHRAbortResult = null; + window.jqXHRTestError = null; + + try { + // This simulates: const jqXHR = Sentry.startSpan(() => $.ajax(...)); + const result = Sentry.startSpan({ name: 'test-jqxhr', op: 'http.client' }, () => { + const dd = createMockJqXHR(); + const hasAbort = typeof dd.abort === 'function'; + const hasStatus = 'status' in dd; + const hasReadyState = 'readyState' in dd; + + console.log('ddhasAbort:', hasAbort, 'hasStatus:', hasStatus, 'hasReadyState:', hasReadyState); + return dd; + }); + + console.log('result from startSpan:', result); + + // Check if extra methods are preserved via Proxy + const hasAbort = typeof result.abort === 'function'; + const hasStatus = 'status' in result; + const hasReadyState = 'readyState' in result; + + console.log('hasAbort:', hasAbort, 'hasStatus:', hasStatus, 'hasReadyState:', hasReadyState); + + if (hasAbort && hasStatus && hasReadyState) { + // Call abort() to test it works + const abortResult = result.abort(); + window.jqXHRMethodsPreserved = true; + window.jqXHRAbortReturnValue = abortResult; + } else { + window.jqXHRMethodsPreserved = false; + } + + // Verify promise functionality still works + try { + await result; + window.jqXHRPromiseResolved = true; + } catch (err) { + window.jqXHRPromiseResolved = false; + } + } catch (error) { + window.jqXHRTestError = error.message; + window.jqXHRMethodsPreserved = false; + } +} + +runTest(); diff --git a/dev-packages/browser-integration-tests/suites/public-api/startSpan/thenable-with-extra-methods/test.ts b/dev-packages/browser-integration-tests/suites/public-api/startSpan/thenable-with-extra-methods/test.ts new file mode 100644 index 000000000000..6c25d2aa774e --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/public-api/startSpan/thenable-with-extra-methods/test.ts @@ -0,0 +1,69 @@ +import { expect } from '@playwright/test'; +import { sentryTest } from '../../../../utils/fixtures'; +import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest } from '../../../../utils/helpers'; + +/** + * These tests verify that thenable objects with extra methods (like jQuery's jqXHR) + * preserve those methods when returned from startSpan(). + * + * Tests the Proxy fix that allows code like this to work: + * const jqXHR = Sentry.startSpan({ name: "test" }, () => $.ajax(...)); + * jqXHR.abort(); // Now works! ✅ + */ + +sentryTest('preserves extra methods on jqXHR-like thenable objects', async ({ getLocalTestUrl, page }) => { + page.on('console', msg => { + console.log(`Console log from page: ${msg.text()}`); + }); + + if (shouldSkipTracingTest()) { + sentryTest.skip(); + } + + const url = await getLocalTestUrl({ testDir: __dirname }); + + // Wait for the transaction to be sent + const transactionPromise = waitForTransactionRequest(page); + + await page.goto(url); + + // Wait for test to complete + await page.waitForTimeout(200); + + // Verify extra methods are preserved + const methodsPreserved = await page.evaluate(() => (window as any).jqXHRMethodsPreserved); + expect(methodsPreserved).toBe(true); + + // Verify abort() was actually called + const abortCalled = await page.evaluate(() => (window as any).jqXHRAbortCalled); + expect(abortCalled).toBe(true); + + // Verify abort() returned the correct value + const abortReturnValue = await page.evaluate(() => (window as any).jqXHRAbortReturnValue); + expect(abortReturnValue).toBe('abort-successful'); + + // Verify no errors occurred + const testError = await page.evaluate(() => (window as any).jqXHRTestError); + expect(testError).toBeNull(); + + // Verify the span was created and sent + const transaction = envelopeRequestParser(await transactionPromise); + expect(transaction.transaction).toBe('test-jqxhr'); + expect(transaction.spans).toBeDefined(); +}); + +sentryTest('preserved methods maintain promise functionality', async ({ getLocalTestUrl, page }) => { + if (shouldSkipTracingTest()) { + sentryTest.skip(); + } + + const url = await getLocalTestUrl({ testDir: __dirname }); + await page.goto(url); + + // Wait for promise to resolve + await page.waitForTimeout(200); + + // Verify promise resolved correctly despite having extra methods + const promiseResolved = await page.evaluate(() => (window as any).jqXHRPromiseResolved); + expect(promiseResolved).toBe(true); +}); diff --git a/packages/core/src/utils/handleCallbackErrors.ts b/packages/core/src/utils/handleCallbackErrors.ts index 1a09e23a40aa..62ccde6eebed 100644 --- a/packages/core/src/utils/handleCallbackErrors.ts +++ b/packages/core/src/utils/handleCallbackErrors.ts @@ -62,7 +62,12 @@ export function handleCallbackErrors< * Maybe handle a promise rejection. * This expects to be given a value that _may_ be a promise, or any other value. * If it is a promise, and it rejects, it will call the `onError` callback. - * Other than this, it will generally return the given value as-is. + * + * For thenable objects with extra methods (like jQuery's jqXHR), + * this function preserves those methods by wrapping the original thenable in a Proxy + * that intercepts .then() calls to apply error handling while forwarding all other + * properties to the original object. + * This allows code like `startSpan(() => $.ajax(...)).abort()` to work correctly. */ function maybeHandlePromiseRejection( value: MaybePromise, @@ -71,22 +76,117 @@ function maybeHandlePromiseRejection( onSuccess: (result: MaybePromise | AwaitedPromise) => void, ): MaybePromise { if (isThenable(value)) { - // @ts-expect-error - the isThenable check returns the "wrong" type here - return value.then( - res => { - onFinally(); - onSuccess(res); - return res; - }, - e => { - onError(e); - onFinally(); - throw e; + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + const hasAbort = typeof value.abort === 'function'; + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + const hasStatus = 'status' in value; + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + const hasReadyState = 'readyState' in value; + console.log('[ORIGINAL] valuehasAbort:', hasAbort, 'hasStatus:', hasStatus, 'hasReadyState:', hasReadyState); + + // Track whether we've already attached handlers to avoid calling callbacks multiple times + let handlersAttached = false; + + // Wrap the original value directly to preserve all its methods + return new Proxy(value, { + get(target, prop, receiver) { + console.log(`[PROXY GET] Accessing property: "${String(prop)}"`); + + // Special handling for .then() - intercept it to add error handling + if (prop === 'then' && typeof target.then === 'function') { + console.log('[PROXY] Intercepting .then() call'); + return function ( + onfulfilled?: ((value: unknown) => unknown) | null, + onrejected?: ((reason: unknown) => unknown) | null, + ) { + // Only attach handlers once to avoid calling callbacks multiple times + if (!handlersAttached) { + handlersAttached = true; + + // Wrap the fulfillment handler to call our callbacks + const wrappedOnFulfilled = onfulfilled + ? (res: unknown) => { + onFinally(); + onSuccess(res as MaybePromise); + return onfulfilled(res); + } + : (res: unknown) => { + onFinally(); + onSuccess(res as MaybePromise); + return res; + }; + + // Wrap the rejection handler to call our callbacks + const wrappedOnRejected = onrejected + ? (err: unknown) => { + onError(err); + onFinally(); + return onrejected(err); + } + : (err: unknown) => { + onError(err); + onFinally(); + throw err; + }; + + // Call the original .then() with our wrapped handlers + const thenResult = target.then.call(target, wrappedOnFulfilled, wrappedOnRejected); + + // CRITICAL: jQuery's .then() returns a new Deferred object without .abort() + // We need to wrap this result in a Proxy that falls back to the original object + return new Proxy(thenResult, { + get(thenTarget, thenProp) { + console.log(`[THEN-PROXY GET] Accessing property: "${String(thenProp)}"`); + // First try the result of .then() + const thenValue = Reflect.get(thenTarget, thenProp, thenTarget); + if (thenValue !== undefined) { + console.log(`[THEN-PROXY] Getting "${String(thenProp)}" from then result:`, typeof thenValue); + return typeof thenValue === 'function' ? thenValue.bind(thenTarget) : thenValue; + } + + // Fall back to the ORIGINAL object for properties like .abort() + const originalValue = Reflect.get(target, thenProp, target); + if (originalValue !== undefined) { + console.log( + `[THEN-PROXY] Getting "${String(thenProp)}" from ORIGINAL object:`, + typeof originalValue, + ); + return typeof originalValue === 'function' ? originalValue.bind(target) : originalValue; + } + + return undefined; + }, + }); + } else { + // Subsequent .then() calls just pass through without wrapping + return target.then.call(target, onfulfilled, onrejected); + } + }; + } + + // For all other properties, forward to the original object + const originalValue = Reflect.get(target, prop, target); + console.log(`[PROXY] Getting property "${String(prop)}" from original:`, typeof originalValue); + + if (originalValue !== undefined) { + // Bind methods to preserve 'this' context + return typeof originalValue === 'function' ? originalValue.bind(target) : originalValue; + } + + return undefined; }, - ); + }); } onFinally(); onSuccess(value); + + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + const hasAbort = typeof value.abort === 'function'; + console.log('[NON-THENABLE] valuehasAbort:', hasAbort); return value; } diff --git a/packages/core/test/lib/utils/handleCallbackErrors-proxy.test.ts b/packages/core/test/lib/utils/handleCallbackErrors-proxy.test.ts new file mode 100644 index 000000000000..534d4eb4085d --- /dev/null +++ b/packages/core/test/lib/utils/handleCallbackErrors-proxy.test.ts @@ -0,0 +1,82 @@ +import { describe, expect, it, vi } from 'vitest'; +import { handleCallbackErrors } from '../../../src/utils/handleCallbackErrors'; + +describe('handleCallbackErrors - Proxy for thenable objects', () => { + it('preserves extra methods on thenable objects (jQuery jqXHR use case)', async () => { + const onError = vi.fn(); + const onFinally = vi.fn(); + + // Mock a JQuery jqXHR-like object with both Promise and XHR methods + let resolvePromise: (value: unknown) => void; + const promise = new Promise(resolve => { + resolvePromise = resolve; + }); + + const mockJqXHR = { + then: promise.then.bind(promise), + catch: promise.catch.bind(promise), + abort: vi.fn(() => 'abort-successful'), + status: 0, + readyState: 1, + responseText: '', + }; + + const fn = vi.fn(() => mockJqXHR); + + const result = handleCallbackErrors(fn, onError, onFinally); + + // Verify the result is thenable + expect(typeof result.then).toBe('function'); + + // Important: Verify extra methods are preserved via Proxy + expect(typeof result.abort).toBe('function'); + expect(typeof result.status).toBe('number'); + expect(typeof result.readyState).toBe('number'); + + const abortResult = result.abort(); + expect(abortResult).toBe('abort-successful'); + expect(mockJqXHR.abort).toHaveBeenCalledTimes(1); + + // Verify promise functionality still works + resolvePromise!({ data: 'test' }); + const promiseResult = await result; + expect(promiseResult).toEqual({ data: 'test' }); + expect(onFinally).toHaveBeenCalledTimes(1); + }); + + it('preserves method binding context', async () => { + const onError = vi.fn(); + + let resolvePromise: (value: unknown) => void; + const promise = new Promise(resolve => { + resolvePromise = resolve; + }); + + const mockJqXHR = { + then: promise.then.bind(promise), + _internalState: 'test-state', + getState: function () { + return this._internalState; + }, + }; + + const fn = vi.fn(() => mockJqXHR); + const result = handleCallbackErrors(fn, onError); + + // Verify method is bound to original object + expect(result.getState()).toBe('test-state'); + + resolvePromise!('done'); + await result; + }); + + it('does not affect non-thenable values', () => { + const onError = vi.fn(); + const fn = vi.fn(() => 'plain-value'); + + const result = handleCallbackErrors(fn, onError); + + expect(result).toBe('plain-value'); + expect(fn).toHaveBeenCalledTimes(1); + }); +}); From 86b9474422ec82b43b0cb0c619717a52104895d5 Mon Sep 17 00:00:00 2001 From: s1gr1d <32902192+s1gr1d@users.noreply.github.com> Date: Wed, 11 Feb 2026 17:45:12 +0100 Subject: [PATCH 2/7] with `has` object trap --- .../thenable-with-extra-methods/subject.js | 103 ++++++++++-------- .../thenable-with-extra-methods/test.ts | 24 ++-- .../core/src/utils/handleCallbackErrors.ts | 49 +++------ .../utils/handleCallbackErrors-proxy.test.ts | 8 +- 4 files changed, 93 insertions(+), 91 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/public-api/startSpan/thenable-with-extra-methods/subject.js b/dev-packages/browser-integration-tests/suites/public-api/startSpan/thenable-with-extra-methods/subject.js index f09573466ce4..35d0adf50ea6 100644 --- a/dev-packages/browser-integration-tests/suites/public-api/startSpan/thenable-with-extra-methods/subject.js +++ b/dev-packages/browser-integration-tests/suites/public-api/startSpan/thenable-with-extra-methods/subject.js @@ -7,36 +7,22 @@ * jqXHR.abort(); // Should work! */ -// Mock a jqXHR-like object (simulates jQuery.ajax() return value) -function createMockJqXHR() { - let resolvePromise; - const promise = new Promise(resolve => { - resolvePromise = resolve; - }); +// Load jQuery from CDN +const script = document.createElement('script'); +script.src = 'https://code.jquery.com/jquery-3.7.1.min.js'; +script.integrity = 'sha256-/JqT3SQfawRcv/BIHPThkBvs0OEvtFFmqPF/lYI/Cxo='; +script.crossOrigin = 'anonymous'; - console.log(''); +script.onload = function () { + runTest(); +}; - // Create an object that has both Promise methods and XHR methods - const mockJqXHR = { - then: promise.then.bind(promise), - catch: promise.catch.bind(promise), - finally: promise.finally.bind(promise), - abort: function () { - window.jqXHRAbortCalled = true; - window.jqXHRAbortResult = 'abort-successful'; - return 'abort-successful'; - }, - // XHR-like properties - status: 0, - readyState: 1, - responseText: '', - }; +script.onerror = function () { + window.jqXHRTestError = 'Failed to load jQuery'; + window.jqXHRMethodsPreserved = false; +}; - // Resolve after a short delay - //setTimeout(() => resolvePromise({ data: 'test response' }), 50); - - return mockJqXHR; -} +document.head.appendChild(script); async function runTest() { window.jqXHRAbortCalled = false; @@ -44,46 +30,69 @@ async function runTest() { window.jqXHRTestError = null; try { - // This simulates: const jqXHR = Sentry.startSpan(() => $.ajax(...)); + if (!window.jQuery) { + throw new Error('jQuery not loaded'); + } + + // Real-world test: Wrap actual jQuery $.ajax() call in startSpan const result = Sentry.startSpan({ name: 'test-jqxhr', op: 'http.client' }, () => { - const dd = createMockJqXHR(); - const hasAbort = typeof dd.abort === 'function'; - const hasStatus = 'status' in dd; - const hasReadyState = 'readyState' in dd; + // Make a real AJAX request with jQuery + const d = window.jQuery.ajax({ + url: 'https://httpbin.org/status/200', + method: 'GET', + timeout: 5000, + }); + // Check if jqXHR methods are preserved + const hasAbort1 = typeof d.abort === 'function'; + const hasStatus1 = 'status' in d; + const hasReadyState1 = 'readyState' in d; - console.log('ddhasAbort:', hasAbort, 'hasStatus:', hasStatus, 'hasReadyState:', hasReadyState); - return dd; - }); + console.log('jqXHR methods preserved:', d.readyState, { hasAbort1, hasStatus1, hasReadyState1 }); - console.log('result from startSpan:', result); + return d; + }); - // Check if extra methods are preserved via Proxy + // Check if jqXHR methods are preserved using 'in' operator (tests has trap) const hasAbort = typeof result.abort === 'function'; - const hasStatus = 'status' in result; const hasReadyState = 'readyState' in result; - console.log('hasAbort:', hasAbort, 'hasStatus:', hasStatus, 'hasReadyState:', hasReadyState); + console.log('Result from startSpan:', result.toString(), Object.keys(result)); + + console.log('jqXHR methods preserved:', { + hasAbort, + hasReadyState, + readyStateValue: result.readyState, + abortType: typeof result.abort, + }); - if (hasAbort && hasStatus && hasReadyState) { + if (hasAbort && hasReadyState) { // Call abort() to test it works - const abortResult = result.abort(); - window.jqXHRMethodsPreserved = true; - window.jqXHRAbortReturnValue = abortResult; + try { + result.abort(); + window.jqXHRAbortCalled = true; + window.jqXHRAbortResult = 'abort-successful'; + window.jqXHRMethodsPreserved = true; + } catch (e) { + window.jqXHRTestError = `abort() failed: ${e.message}`; + window.jqXHRMethodsPreserved = false; + } } else { window.jqXHRMethodsPreserved = false; + window.jqXHRTestError = 'jqXHR methods not preserved'; } - // Verify promise functionality still works + // Since we aborted the request, it should be rejected try { await result; - window.jqXHRPromiseResolved = true; + window.jqXHRPromiseResolved = true; // Unexpected } catch (err) { + // Expected: aborted request rejects window.jqXHRPromiseResolved = false; + window.jqXHRPromiseRejected = true; } } catch (error) { + console.error('Test error:', error); window.jqXHRTestError = error.message; window.jqXHRMethodsPreserved = false; } } - -runTest(); diff --git a/dev-packages/browser-integration-tests/suites/public-api/startSpan/thenable-with-extra-methods/test.ts b/dev-packages/browser-integration-tests/suites/public-api/startSpan/thenable-with-extra-methods/test.ts index 6c25d2aa774e..c02516a20dc3 100644 --- a/dev-packages/browser-integration-tests/suites/public-api/startSpan/thenable-with-extra-methods/test.ts +++ b/dev-packages/browser-integration-tests/suites/public-api/startSpan/thenable-with-extra-methods/test.ts @@ -11,7 +11,7 @@ import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest * jqXHR.abort(); // Now works! ✅ */ -sentryTest('preserves extra methods on jqXHR-like thenable objects', async ({ getLocalTestUrl, page }) => { +sentryTest('preserves extra methods on real jQuery jqXHR objects', async ({ getLocalTestUrl, page }) => { page.on('console', msg => { console.log(`Console log from page: ${msg.text()}`); }); @@ -27,8 +27,8 @@ sentryTest('preserves extra methods on jqXHR-like thenable objects', async ({ ge await page.goto(url); - // Wait for test to complete - await page.waitForTimeout(200); + // Wait for jQuery to load and test to complete + await page.waitForTimeout(1000); // Verify extra methods are preserved const methodsPreserved = await page.evaluate(() => (window as any).jqXHRMethodsPreserved); @@ -38,8 +38,8 @@ sentryTest('preserves extra methods on jqXHR-like thenable objects', async ({ ge const abortCalled = await page.evaluate(() => (window as any).jqXHRAbortCalled); expect(abortCalled).toBe(true); - // Verify abort() returned the correct value - const abortReturnValue = await page.evaluate(() => (window as any).jqXHRAbortReturnValue); + // Verify abort() returned successfully + const abortReturnValue = await page.evaluate(() => (window as any).jqXHRAbortResult); expect(abortReturnValue).toBe('abort-successful'); // Verify no errors occurred @@ -52,7 +52,7 @@ sentryTest('preserves extra methods on jqXHR-like thenable objects', async ({ ge expect(transaction.spans).toBeDefined(); }); -sentryTest('preserved methods maintain promise functionality', async ({ getLocalTestUrl, page }) => { +sentryTest('aborted request rejects promise correctly', async ({ getLocalTestUrl, page }) => { if (shouldSkipTracingTest()) { sentryTest.skip(); } @@ -60,10 +60,14 @@ sentryTest('preserved methods maintain promise functionality', async ({ getLocal const url = await getLocalTestUrl({ testDir: __dirname }); await page.goto(url); - // Wait for promise to resolve - await page.waitForTimeout(200); + // Wait for jQuery to load and test to complete + await page.waitForTimeout(1000); - // Verify promise resolved correctly despite having extra methods + // Verify the aborted request was rejected (not resolved) + const promiseRejected = await page.evaluate(() => (window as any).jqXHRPromiseRejected); + expect(promiseRejected).toBe(true); + + // Should NOT have resolved const promiseResolved = await page.evaluate(() => (window as any).jqXHRPromiseResolved); - expect(promiseResolved).toBe(true); + expect(promiseResolved).toBe(false); }); diff --git a/packages/core/src/utils/handleCallbackErrors.ts b/packages/core/src/utils/handleCallbackErrors.ts index 62ccde6eebed..e3cdd860ad0e 100644 --- a/packages/core/src/utils/handleCallbackErrors.ts +++ b/packages/core/src/utils/handleCallbackErrors.ts @@ -76,28 +76,14 @@ function maybeHandlePromiseRejection( onSuccess: (result: MaybePromise | AwaitedPromise) => void, ): MaybePromise { if (isThenable(value)) { - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore - const hasAbort = typeof value.abort === 'function'; - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore - const hasStatus = 'status' in value; - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore - const hasReadyState = 'readyState' in value; - console.log('[ORIGINAL] valuehasAbort:', hasAbort, 'hasStatus:', hasStatus, 'hasReadyState:', hasReadyState); - // Track whether we've already attached handlers to avoid calling callbacks multiple times let handlersAttached = false; - // Wrap the original value directly to preserve all its methods + // 1. Wrap the original thenable in a Proxy to preserve all its methods return new Proxy(value, { get(target, prop, receiver) { - console.log(`[PROXY GET] Accessing property: "${String(prop)}"`); - // Special handling for .then() - intercept it to add error handling if (prop === 'then' && typeof target.then === 'function') { - console.log('[PROXY] Intercepting .then() call'); return function ( onfulfilled?: ((value: unknown) => unknown) | null, onrejected?: ((reason: unknown) => unknown) | null, @@ -135,33 +121,32 @@ function maybeHandlePromiseRejection( // Call the original .then() with our wrapped handlers const thenResult = target.then.call(target, wrappedOnFulfilled, wrappedOnRejected); - // CRITICAL: jQuery's .then() returns a new Deferred object without .abort() - // We need to wrap this result in a Proxy that falls back to the original object + // 2. Some thenable implementations (like jQuery) return a new object from .then() + // that doesn't include custom properties from the original (like .abort()). + // We wrap the result in another Proxy to preserve access to those properties. return new Proxy(thenResult, { get(thenTarget, thenProp) { - console.log(`[THEN-PROXY GET] Accessing property: "${String(thenProp)}"`); - // First try the result of .then() + // First try to get the property from the .then() result const thenValue = Reflect.get(thenTarget, thenProp, thenTarget); if (thenValue !== undefined) { - console.log(`[THEN-PROXY] Getting "${String(thenProp)}" from then result:`, typeof thenValue); return typeof thenValue === 'function' ? thenValue.bind(thenTarget) : thenValue; } - // Fall back to the ORIGINAL object for properties like .abort() + // Fall back to the original object for properties like .abort() const originalValue = Reflect.get(target, thenProp, target); if (originalValue !== undefined) { - console.log( - `[THEN-PROXY] Getting "${String(thenProp)}" from ORIGINAL object:`, - typeof originalValue, - ); return typeof originalValue === 'function' ? originalValue.bind(target) : originalValue; } return undefined; }, + has(thenTarget, thenProp) { + // Check if property exists in either the .then() result or the original object + return thenProp in thenTarget || thenProp in (target as object); + }, }); } else { - // Subsequent .then() calls just pass through without wrapping + // Subsequent .then() calls pass through without additional wrapping return target.then.call(target, onfulfilled, onrejected); } }; @@ -169,8 +154,6 @@ function maybeHandlePromiseRejection( // For all other properties, forward to the original object const originalValue = Reflect.get(target, prop, target); - console.log(`[PROXY] Getting property "${String(prop)}" from original:`, typeof originalValue); - if (originalValue !== undefined) { // Bind methods to preserve 'this' context return typeof originalValue === 'function' ? originalValue.bind(target) : originalValue; @@ -178,15 +161,15 @@ function maybeHandlePromiseRejection( return undefined; }, + has(target, prop) { + // Check if property exists in the original object + return prop in (target as object); + }, }); } + // Non-thenable value - call callbacks immediately and return as-is onFinally(); onSuccess(value); - - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore - const hasAbort = typeof value.abort === 'function'; - console.log('[NON-THENABLE] valuehasAbort:', hasAbort); return value; } diff --git a/packages/core/test/lib/utils/handleCallbackErrors-proxy.test.ts b/packages/core/test/lib/utils/handleCallbackErrors-proxy.test.ts index 534d4eb4085d..a50021e75d3b 100644 --- a/packages/core/test/lib/utils/handleCallbackErrors-proxy.test.ts +++ b/packages/core/test/lib/utils/handleCallbackErrors-proxy.test.ts @@ -28,11 +28,17 @@ describe('handleCallbackErrors - Proxy for thenable objects', () => { // Verify the result is thenable expect(typeof result.then).toBe('function'); - // Important: Verify extra methods are preserved via Proxy + // Important: Verify extra methods are preserved via Proxy (property access) expect(typeof result.abort).toBe('function'); expect(typeof result.status).toBe('number'); expect(typeof result.readyState).toBe('number'); + // Verify the 'in' operator works correctly (has trap) + expect('abort' in result).toBe(true); + expect('status' in result).toBe(true); + expect('readyState' in result).toBe(true); + expect('then' in result).toBe(true); + const abortResult = result.abort(); expect(abortResult).toBe('abort-successful'); expect(mockJqXHR.abort).toHaveBeenCalledTimes(1); From 316edc2127b0bdcaa28a3534b2759d8c1533c685 Mon Sep 17 00:00:00 2001 From: s1gr1d <32902192+s1gr1d@users.noreply.github.com> Date: Thu, 12 Feb 2026 09:16:32 +0100 Subject: [PATCH 3/7] remove proxy wrapping --- .../browser-integration-tests/package.json | 2 +- .../thenable-with-extra-methods/subject.js | 7 +- .../core/src/asyncContext/stackStrategy.ts | 6 +- .../core/src/utils/handleCallbackErrors.ts | 99 +++---------------- 4 files changed, 21 insertions(+), 93 deletions(-) diff --git a/dev-packages/browser-integration-tests/package.json b/dev-packages/browser-integration-tests/package.json index f0d8272a10b4..62b6893777d4 100644 --- a/dev-packages/browser-integration-tests/package.json +++ b/dev-packages/browser-integration-tests/package.json @@ -16,7 +16,7 @@ "postinstall": "yarn install-browsers", "pretest": "yarn clean && yarn type-check", "test": "yarn test:all --project='chromium'", - "test:all": "npx playwright test -c playwright.browser.config.ts", + "test:all": "npx playwright test -c playwright.browser.config.ts -g 'thenable'", "test:bundle": "PW_BUNDLE=bundle yarn test", "test:bundle:min": "PW_BUNDLE=bundle_min yarn test", "test:bundle:logs_metrics": "PW_BUNDLE=bundle_logs_metrics yarn test", diff --git a/dev-packages/browser-integration-tests/suites/public-api/startSpan/thenable-with-extra-methods/subject.js b/dev-packages/browser-integration-tests/suites/public-api/startSpan/thenable-with-extra-methods/subject.js index 35d0adf50ea6..3e43239c2b5c 100644 --- a/dev-packages/browser-integration-tests/suites/public-api/startSpan/thenable-with-extra-methods/subject.js +++ b/dev-packages/browser-integration-tests/suites/public-api/startSpan/thenable-with-extra-methods/subject.js @@ -47,6 +47,8 @@ async function runTest() { const hasStatus1 = 'status' in d; const hasReadyState1 = 'readyState' in d; + console.log('[AJAX CALL] jqXHR object:', Object.keys(d)); + console.log('jqXHR methods preserved:', d.readyState, { hasAbort1, hasStatus1, hasReadyState1 }); return d; @@ -56,7 +58,7 @@ async function runTest() { const hasAbort = typeof result.abort === 'function'; const hasReadyState = 'readyState' in result; - console.log('Result from startSpan:', result.toString(), Object.keys(result)); + console.log('Result object keys:', Object.keys(result)); console.log('jqXHR methods preserved:', { hasAbort, @@ -65,7 +67,7 @@ async function runTest() { abortType: typeof result.abort, }); - if (hasAbort && hasReadyState) { + if (true || (hasAbort && hasReadyState)) { // Call abort() to test it works try { result.abort(); @@ -73,6 +75,7 @@ async function runTest() { window.jqXHRAbortResult = 'abort-successful'; window.jqXHRMethodsPreserved = true; } catch (e) { + console.log('abort() threw an error:', e); window.jqXHRTestError = `abort() failed: ${e.message}`; window.jqXHRMethodsPreserved = false; } diff --git a/packages/core/src/asyncContext/stackStrategy.ts b/packages/core/src/asyncContext/stackStrategy.ts index 87dc534fc636..02e8dd374df6 100644 --- a/packages/core/src/asyncContext/stackStrategy.ts +++ b/packages/core/src/asyncContext/stackStrategy.ts @@ -52,8 +52,8 @@ export class AsyncContextStack { } if (isThenable(maybePromiseResult)) { - // @ts-expect-error - isThenable returns the wrong type - return maybePromiseResult.then( + // Attach handlers but still return original promise + maybePromiseResult.then( res => { this._popScope(); return res; @@ -63,6 +63,8 @@ export class AsyncContextStack { throw e; }, ); + + return maybePromiseResult; } this._popScope(); diff --git a/packages/core/src/utils/handleCallbackErrors.ts b/packages/core/src/utils/handleCallbackErrors.ts index e3cdd860ad0e..a8c9177c5ac0 100644 --- a/packages/core/src/utils/handleCallbackErrors.ts +++ b/packages/core/src/utils/handleCallbackErrors.ts @@ -76,96 +76,19 @@ function maybeHandlePromiseRejection( onSuccess: (result: MaybePromise | AwaitedPromise) => void, ): MaybePromise { if (isThenable(value)) { - // Track whether we've already attached handlers to avoid calling callbacks multiple times - let handlersAttached = false; - - // 1. Wrap the original thenable in a Proxy to preserve all its methods - return new Proxy(value, { - get(target, prop, receiver) { - // Special handling for .then() - intercept it to add error handling - if (prop === 'then' && typeof target.then === 'function') { - return function ( - onfulfilled?: ((value: unknown) => unknown) | null, - onrejected?: ((reason: unknown) => unknown) | null, - ) { - // Only attach handlers once to avoid calling callbacks multiple times - if (!handlersAttached) { - handlersAttached = true; - - // Wrap the fulfillment handler to call our callbacks - const wrappedOnFulfilled = onfulfilled - ? (res: unknown) => { - onFinally(); - onSuccess(res as MaybePromise); - return onfulfilled(res); - } - : (res: unknown) => { - onFinally(); - onSuccess(res as MaybePromise); - return res; - }; - - // Wrap the rejection handler to call our callbacks - const wrappedOnRejected = onrejected - ? (err: unknown) => { - onError(err); - onFinally(); - return onrejected(err); - } - : (err: unknown) => { - onError(err); - onFinally(); - throw err; - }; - - // Call the original .then() with our wrapped handlers - const thenResult = target.then.call(target, wrappedOnFulfilled, wrappedOnRejected); - - // 2. Some thenable implementations (like jQuery) return a new object from .then() - // that doesn't include custom properties from the original (like .abort()). - // We wrap the result in another Proxy to preserve access to those properties. - return new Proxy(thenResult, { - get(thenTarget, thenProp) { - // First try to get the property from the .then() result - const thenValue = Reflect.get(thenTarget, thenProp, thenTarget); - if (thenValue !== undefined) { - return typeof thenValue === 'function' ? thenValue.bind(thenTarget) : thenValue; - } - - // Fall back to the original object for properties like .abort() - const originalValue = Reflect.get(target, thenProp, target); - if (originalValue !== undefined) { - return typeof originalValue === 'function' ? originalValue.bind(target) : originalValue; - } - - return undefined; - }, - has(thenTarget, thenProp) { - // Check if property exists in either the .then() result or the original object - return thenProp in thenTarget || thenProp in (target as object); - }, - }); - } else { - // Subsequent .then() calls pass through without additional wrapping - return target.then.call(target, onfulfilled, onrejected); - } - }; - } - - // For all other properties, forward to the original object - const originalValue = Reflect.get(target, prop, target); - if (originalValue !== undefined) { - // Bind methods to preserve 'this' context - return typeof originalValue === 'function' ? originalValue.bind(target) : originalValue; - } - - return undefined; + // Attach handlers but still return original promise + value.then( + res => { + onFinally(); + onSuccess(res); }, - has(target, prop) { - // Check if property exists in the original object - return prop in (target as object); + err => { + onError(err); + onFinally(); }, - }); + ); + + return value; } // Non-thenable value - call callbacks immediately and return as-is From 137636b40724b7642b260f9a20501dae79b4309e Mon Sep 17 00:00:00 2001 From: s1gr1d <32902192+s1gr1d@users.noreply.github.com> Date: Thu, 12 Feb 2026 12:04:05 +0100 Subject: [PATCH 4/7] add tracing unit tests --- packages/core/test/lib/tracing/trace.test.ts | 26 ++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/packages/core/test/lib/tracing/trace.test.ts b/packages/core/test/lib/tracing/trace.test.ts index 271cd669d56c..e7ea8bc8fa7e 100644 --- a/packages/core/test/lib/tracing/trace.test.ts +++ b/packages/core/test/lib/tracing/trace.test.ts @@ -210,6 +210,32 @@ describe('startSpan', () => { }); }); + describe('AsyncContext withScope promise integrity behavior', () => { + it('returns the original promise instance', async () => { + const original = Promise.resolve(42); + const result = startSpan({}, () => original); + expect(result).toBe(original); // New behavior + }); + + it('returns same instance on multiple calls', () => { + const p = Promise.resolve(1); + const result1 = startSpan({}, () => p); + const result2 = startSpan({}, () => p); + expect(result1).toBe(result2); + }); + + it('preserves custom thenable methods', async () => { + const jqXHR = { + then: Promise.resolve(1).then.bind(Promise.resolve(1)), + abort: vi.fn(), + }; + const result = startSpan({}, () => jqXHR); + expect(typeof result.abort).toBe('function'); + result.abort(); + expect(jqXHR.abort).toHaveBeenCalled(); + }); + }); + it('returns a non recording span if tracing is disabled', () => { const options = getDefaultTestClientOptions({}); client = new TestClient(options); From 69f840bd977d9fb50766b468d7d006992e74ca5e Mon Sep 17 00:00:00 2001 From: s1gr1d <32902192+s1gr1d@users.noreply.github.com> Date: Thu, 12 Feb 2026 15:35:19 +0100 Subject: [PATCH 5/7] clean up test cases --- .../thenable-with-extra-methods/subject.js | 30 +------ .../thenable-with-extra-methods/test.ts | 26 +----- .../utils/handleCallbackErrors-proxy.test.ts | 88 ------------------- 3 files changed, 6 insertions(+), 138 deletions(-) delete mode 100644 packages/core/test/lib/utils/handleCallbackErrors-proxy.test.ts diff --git a/dev-packages/browser-integration-tests/suites/public-api/startSpan/thenable-with-extra-methods/subject.js b/dev-packages/browser-integration-tests/suites/public-api/startSpan/thenable-with-extra-methods/subject.js index 3e43239c2b5c..6a0a6c4cae5e 100644 --- a/dev-packages/browser-integration-tests/suites/public-api/startSpan/thenable-with-extra-methods/subject.js +++ b/dev-packages/browser-integration-tests/suites/public-api/startSpan/thenable-with-extra-methods/subject.js @@ -2,9 +2,9 @@ * Test that verifies thenable objects with extra methods (like jQuery's jqXHR) * preserve those methods when returned from Sentry.startSpan(). * - * This tests the Proxy fix for the GitHub issue where: + * Example case: * const jqXHR = Sentry.startSpan({ name: "test" }, () => $.ajax(...)); - * jqXHR.abort(); // Should work! + * jqXHR.abort(); // Should work and not throw an error because of missing abort() method */ // Load jQuery from CDN @@ -34,41 +34,19 @@ async function runTest() { throw new Error('jQuery not loaded'); } - // Real-world test: Wrap actual jQuery $.ajax() call in startSpan const result = Sentry.startSpan({ name: 'test-jqxhr', op: 'http.client' }, () => { // Make a real AJAX request with jQuery - const d = window.jQuery.ajax({ + return window.jQuery.ajax({ url: 'https://httpbin.org/status/200', method: 'GET', timeout: 5000, }); - // Check if jqXHR methods are preserved - const hasAbort1 = typeof d.abort === 'function'; - const hasStatus1 = 'status' in d; - const hasReadyState1 = 'readyState' in d; - - console.log('[AJAX CALL] jqXHR object:', Object.keys(d)); - - console.log('jqXHR methods preserved:', d.readyState, { hasAbort1, hasStatus1, hasReadyState1 }); - - return d; }); - // Check if jqXHR methods are preserved using 'in' operator (tests has trap) const hasAbort = typeof result.abort === 'function'; const hasReadyState = 'readyState' in result; - console.log('Result object keys:', Object.keys(result)); - - console.log('jqXHR methods preserved:', { - hasAbort, - hasReadyState, - readyStateValue: result.readyState, - abortType: typeof result.abort, - }); - - if (true || (hasAbort && hasReadyState)) { - // Call abort() to test it works + if (hasAbort && hasReadyState) { try { result.abort(); window.jqXHRAbortCalled = true; diff --git a/dev-packages/browser-integration-tests/suites/public-api/startSpan/thenable-with-extra-methods/test.ts b/dev-packages/browser-integration-tests/suites/public-api/startSpan/thenable-with-extra-methods/test.ts index c02516a20dc3..44008a93a3e9 100644 --- a/dev-packages/browser-integration-tests/suites/public-api/startSpan/thenable-with-extra-methods/test.ts +++ b/dev-packages/browser-integration-tests/suites/public-api/startSpan/thenable-with-extra-methods/test.ts @@ -2,51 +2,31 @@ import { expect } from '@playwright/test'; import { sentryTest } from '../../../../utils/fixtures'; import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest } from '../../../../utils/helpers'; -/** - * These tests verify that thenable objects with extra methods (like jQuery's jqXHR) - * preserve those methods when returned from startSpan(). - * - * Tests the Proxy fix that allows code like this to work: - * const jqXHR = Sentry.startSpan({ name: "test" }, () => $.ajax(...)); - * jqXHR.abort(); // Now works! ✅ - */ - sentryTest('preserves extra methods on real jQuery jqXHR objects', async ({ getLocalTestUrl, page }) => { - page.on('console', msg => { - console.log(`Console log from page: ${msg.text()}`); - }); - if (shouldSkipTracingTest()) { sentryTest.skip(); } const url = await getLocalTestUrl({ testDir: __dirname }); - - // Wait for the transaction to be sent const transactionPromise = waitForTransactionRequest(page); await page.goto(url); - // Wait for jQuery to load and test to complete + // Wait for jQuery to load await page.waitForTimeout(1000); - // Verify extra methods are preserved const methodsPreserved = await page.evaluate(() => (window as any).jqXHRMethodsPreserved); expect(methodsPreserved).toBe(true); - // Verify abort() was actually called const abortCalled = await page.evaluate(() => (window as any).jqXHRAbortCalled); expect(abortCalled).toBe(true); - // Verify abort() returned successfully const abortReturnValue = await page.evaluate(() => (window as any).jqXHRAbortResult); expect(abortReturnValue).toBe('abort-successful'); - // Verify no errors occurred const testError = await page.evaluate(() => (window as any).jqXHRTestError); expect(testError).toBeNull(); - // Verify the span was created and sent const transaction = envelopeRequestParser(await transactionPromise); expect(transaction.transaction).toBe('test-jqxhr'); expect(transaction.spans).toBeDefined(); @@ -60,14 +40,12 @@ sentryTest('aborted request rejects promise correctly', async ({ getLocalTestUrl const url = await getLocalTestUrl({ testDir: __dirname }); await page.goto(url); - // Wait for jQuery to load and test to complete + // Wait for jQuery to load await page.waitForTimeout(1000); - // Verify the aborted request was rejected (not resolved) const promiseRejected = await page.evaluate(() => (window as any).jqXHRPromiseRejected); expect(promiseRejected).toBe(true); - // Should NOT have resolved const promiseResolved = await page.evaluate(() => (window as any).jqXHRPromiseResolved); expect(promiseResolved).toBe(false); }); diff --git a/packages/core/test/lib/utils/handleCallbackErrors-proxy.test.ts b/packages/core/test/lib/utils/handleCallbackErrors-proxy.test.ts deleted file mode 100644 index a50021e75d3b..000000000000 --- a/packages/core/test/lib/utils/handleCallbackErrors-proxy.test.ts +++ /dev/null @@ -1,88 +0,0 @@ -import { describe, expect, it, vi } from 'vitest'; -import { handleCallbackErrors } from '../../../src/utils/handleCallbackErrors'; - -describe('handleCallbackErrors - Proxy for thenable objects', () => { - it('preserves extra methods on thenable objects (jQuery jqXHR use case)', async () => { - const onError = vi.fn(); - const onFinally = vi.fn(); - - // Mock a JQuery jqXHR-like object with both Promise and XHR methods - let resolvePromise: (value: unknown) => void; - const promise = new Promise(resolve => { - resolvePromise = resolve; - }); - - const mockJqXHR = { - then: promise.then.bind(promise), - catch: promise.catch.bind(promise), - abort: vi.fn(() => 'abort-successful'), - status: 0, - readyState: 1, - responseText: '', - }; - - const fn = vi.fn(() => mockJqXHR); - - const result = handleCallbackErrors(fn, onError, onFinally); - - // Verify the result is thenable - expect(typeof result.then).toBe('function'); - - // Important: Verify extra methods are preserved via Proxy (property access) - expect(typeof result.abort).toBe('function'); - expect(typeof result.status).toBe('number'); - expect(typeof result.readyState).toBe('number'); - - // Verify the 'in' operator works correctly (has trap) - expect('abort' in result).toBe(true); - expect('status' in result).toBe(true); - expect('readyState' in result).toBe(true); - expect('then' in result).toBe(true); - - const abortResult = result.abort(); - expect(abortResult).toBe('abort-successful'); - expect(mockJqXHR.abort).toHaveBeenCalledTimes(1); - - // Verify promise functionality still works - resolvePromise!({ data: 'test' }); - const promiseResult = await result; - expect(promiseResult).toEqual({ data: 'test' }); - expect(onFinally).toHaveBeenCalledTimes(1); - }); - - it('preserves method binding context', async () => { - const onError = vi.fn(); - - let resolvePromise: (value: unknown) => void; - const promise = new Promise(resolve => { - resolvePromise = resolve; - }); - - const mockJqXHR = { - then: promise.then.bind(promise), - _internalState: 'test-state', - getState: function () { - return this._internalState; - }, - }; - - const fn = vi.fn(() => mockJqXHR); - const result = handleCallbackErrors(fn, onError); - - // Verify method is bound to original object - expect(result.getState()).toBe('test-state'); - - resolvePromise!('done'); - await result; - }); - - it('does not affect non-thenable values', () => { - const onError = vi.fn(); - const fn = vi.fn(() => 'plain-value'); - - const result = handleCallbackErrors(fn, onError); - - expect(result).toBe('plain-value'); - expect(fn).toHaveBeenCalledTimes(1); - }); -}); From b64405a3fd3390d96c41201f233a114d4a364879 Mon Sep 17 00:00:00 2001 From: s1gr1d <32902192+s1gr1d@users.noreply.github.com> Date: Thu, 12 Feb 2026 15:59:39 +0100 Subject: [PATCH 6/7] remove throw error --- dev-packages/browser-integration-tests/package.json | 2 +- packages/core/src/asyncContext/stackStrategy.ts | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/dev-packages/browser-integration-tests/package.json b/dev-packages/browser-integration-tests/package.json index 62b6893777d4..f0d8272a10b4 100644 --- a/dev-packages/browser-integration-tests/package.json +++ b/dev-packages/browser-integration-tests/package.json @@ -16,7 +16,7 @@ "postinstall": "yarn install-browsers", "pretest": "yarn clean && yarn type-check", "test": "yarn test:all --project='chromium'", - "test:all": "npx playwright test -c playwright.browser.config.ts -g 'thenable'", + "test:all": "npx playwright test -c playwright.browser.config.ts", "test:bundle": "PW_BUNDLE=bundle yarn test", "test:bundle:min": "PW_BUNDLE=bundle_min yarn test", "test:bundle:logs_metrics": "PW_BUNDLE=bundle_logs_metrics yarn test", diff --git a/packages/core/src/asyncContext/stackStrategy.ts b/packages/core/src/asyncContext/stackStrategy.ts index 02e8dd374df6..da945aa46138 100644 --- a/packages/core/src/asyncContext/stackStrategy.ts +++ b/packages/core/src/asyncContext/stackStrategy.ts @@ -58,9 +58,10 @@ export class AsyncContextStack { this._popScope(); return res; }, - e => { + _e => { this._popScope(); - throw e; + // We don't re-throw the error here because the caller already receives the original rejection, and it's being handled by the caller of withScope. + // Re-throwing it here would cause unhandled promise rejections. }, ); From 58c695d4d8475aa41ea10b77efa7821687d27bc4 Mon Sep 17 00:00:00 2001 From: isaacs Date: Fri, 13 Feb 2026 11:35:18 -0800 Subject: [PATCH 7/7] wip: Copy properties onto chained promises This might be an acceptable compromise, so long as it doesn't blow up our bundle size too much. Copy properties from the original promise onto the chained tracker, so that we can return something that can inspect the error, does not obscure `unhandledrejection`, *and* supports jQuery's extended PromiseLike objects. --- .../core/src/asyncContext/stackStrategy.ts | 20 +++------ .../src/utils/chain-and-copy-promiselike.ts | 43 +++++++++++++++++++ .../core/src/utils/handleCallbackErrors.ts | 27 ++++++------ 3 files changed, 62 insertions(+), 28 deletions(-) create mode 100644 packages/core/src/utils/chain-and-copy-promiselike.ts diff --git a/packages/core/src/asyncContext/stackStrategy.ts b/packages/core/src/asyncContext/stackStrategy.ts index da945aa46138..36c1d2127530 100644 --- a/packages/core/src/asyncContext/stackStrategy.ts +++ b/packages/core/src/asyncContext/stackStrategy.ts @@ -1,6 +1,7 @@ import type { Client } from '../client'; import { getDefaultCurrentScope, getDefaultIsolationScope } from '../defaultScopes'; import { Scope } from '../scope'; +import { chainAndCopyPromiseLike } from '../utils/chain-and-copy-promiselike'; import { isThenable } from '../utils/is'; import { getMainCarrier, getSentryCarrier } from './../carrier'; import type { AsyncContextStrategy } from './types'; @@ -52,20 +53,11 @@ export class AsyncContextStack { } if (isThenable(maybePromiseResult)) { - // Attach handlers but still return original promise - maybePromiseResult.then( - res => { - this._popScope(); - return res; - }, - _e => { - this._popScope(); - // We don't re-throw the error here because the caller already receives the original rejection, and it's being handled by the caller of withScope. - // Re-throwing it here would cause unhandled promise rejections. - }, - ); - - return maybePromiseResult; + return chainAndCopyPromiseLike( + maybePromiseResult as PromiseLike> & Record, + () => this._popScope(), + () => this._popScope(), + ) as T; } this._popScope(); diff --git a/packages/core/src/utils/chain-and-copy-promiselike.ts b/packages/core/src/utils/chain-and-copy-promiselike.ts new file mode 100644 index 000000000000..d0cc130d4f3e --- /dev/null +++ b/packages/core/src/utils/chain-and-copy-promiselike.ts @@ -0,0 +1,43 @@ +/** + * Copy the properties from a decorated promiselike object onto its chained + * actual promise. + */ +export const chainAndCopyPromiseLike = & Record>( + original: T, + onSuccess: (value: V) => void, + onError: (e: unknown) => void, +): T => { + return copyProps( + original, + original.then( + value => { + onSuccess(value); + return value; + }, + err => { + onError(err); + throw err; + }, + ), + ) as T; +}; + +// eslint-disable-next-line @typescript-eslint/no-explicit-any +const copyProps = >(original: T, chained: T): T => { + for (const key in original) { + if (key in chained) continue; + const value = original[key]; + if (typeof value === 'function') { + Object.defineProperty(chained, key, { + value: (...args: unknown[]) => value.apply(original, args), + enumerable: true, + configurable: true, + writable: true, + }); + } else { + (chained as Record)[key] = value; + } + } + + return chained; +}; diff --git a/packages/core/src/utils/handleCallbackErrors.ts b/packages/core/src/utils/handleCallbackErrors.ts index a8c9177c5ac0..72fe475f7e4f 100644 --- a/packages/core/src/utils/handleCallbackErrors.ts +++ b/packages/core/src/utils/handleCallbackErrors.ts @@ -1,4 +1,5 @@ import { isThenable } from '../utils/is'; +import { chainAndCopyPromiseLike } from '../utils/chain-and-copy-promiselike'; /* eslint-disable */ // Vendor "Awaited" in to be TS 3.8 compatible @@ -76,23 +77,21 @@ function maybeHandlePromiseRejection( onSuccess: (result: MaybePromise | AwaitedPromise) => void, ): MaybePromise { if (isThenable(value)) { - // Attach handlers but still return original promise - value.then( - res => { + return chainAndCopyPromiseLike( + value as MaybePromise & PromiseLike> & Record, + (result) => { onFinally(); - onSuccess(res); + onSuccess(result as Awaited); }, - err => { - onError(err); - onFinally(); + (err) => { + onError(err) + onFinally() }, - ); - - return value; + ) as MaybePromise; + } else { + // Non-thenable value - call callbacks immediately and return as-is + onFinally(); + onSuccess(value); } - - // Non-thenable value - call callbacks immediately and return as-is - onFinally(); - onSuccess(value); return value; }