Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(core): Add safeCallback helper for isolating user-provided callbacks#23760
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
Changes from all commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| import { DEBUG_BUILD } from '../debug-build'; | ||
| import { debug } from './debug-logger'; | ||
| import { isThenable } from './is'; | ||
| /** | ||
| * Invokes a user-provided callback (e.g. `beforeSend`, `tracesSampler`, an integration hook) so that | ||
| * neither a synchronous throw nor a rejected promise escapes into the caller. On failure the error is | ||
| * logged and `fallback(error)` supplies the result instead. | ||
| * | ||
| * Not for `startSpan` bodies: those must re-throw and are handled by `handleCallbackErrors`. | ||
| * | ||
| * @param message - Logged via `debug.error` together with the error. Pass it as `DEBUG_BUILD ? '...' : ''` | ||
| * so the string is tree-shaken from non-debug bundles. | ||
| * @param fn - Invokes the callback. | ||
| * @param fallback - Produces the result to use when the callback throws or rejects. | ||
| */ | ||
| export function safeCallback<T>(message: string, fn: () => T, fallback: (error: unknown) => T): T { | ||
| let result: T; | ||
| try { | ||
| result = fn(); | ||
| } catch (error) { | ||
| return recover(message, error, fallback); | ||
| } | ||
| if (isThenable(result)) { | ||
| return result.then(undefined, (error: unknown) => recover(message, error, fallback)) as T; | ||
| } | ||
| return result; | ||
| } | ||
| function recover<T>(message: string, error: unknown, fallback: (error: unknown) => T): T { | ||
| DEBUG_BUILD && debug.error(message, error); | ||
Member 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 think the flag is correct here but the bot review is right: We no longer tree-shake out the actual string passed to this function if DEBUG_BUILD is false. We can still add a ternary to the call-sitest to enable tree shaking, something like safeCallback(DEBUG_BUILD ? 'full warning/error message' : '',()=>{},()=>{}) | ||
| return fallback(error); | ||
cursor[bot] marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| import { afterEach, describe, expect, it, vi } from 'vitest'; | ||
| import { debug } from '../../../src/utils/debug-logger'; | ||
| import { safeCallback } from '../../../src/utils/safeCallback'; | ||
| describe('safeCallback', () => { | ||
| const debugErrorSpy = vi.spyOn(debug, 'error').mockImplementation(() => undefined); | ||
| afterEach(() => { | ||
| debugErrorSpy.mockClear(); | ||
| }); | ||
| it('returns the result of a sync callback', () => { | ||
| const fallback = vi.fn(() => 'fallback'); | ||
| expect(safeCallback('callback threw:', () => 'value', fallback)).toBe('value'); | ||
| expect(fallback).not.toHaveBeenCalled(); | ||
| expect(debugErrorSpy).not.toHaveBeenCalled(); | ||
| }); | ||
| it('returns the fallback and logs when a sync callback throws', () => { | ||
| const error = new Error('boom'); | ||
| const fallback = vi.fn(() => 'fallback'); | ||
| expect( | ||
| safeCallback( | ||
| 'callback threw:', | ||
| () => { | ||
| throw error; | ||
| }, | ||
| fallback, | ||
| ), | ||
| ).toBe('fallback'); | ||
| expect(fallback).toHaveBeenCalledWith(error); | ||
| expect(debugErrorSpy).toHaveBeenCalledWith('callback threw:', error); | ||
| }); | ||
| it('resolves to the result of an async callback', async () => { | ||
| const fallback = vi.fn(async () => 'fallback'); | ||
| const result = safeCallback('callback threw:', async () => 'value', fallback); | ||
| expect(result).toBeInstanceOf(Promise); | ||
| await expect(result).resolves.toBe('value'); | ||
| expect(fallback).not.toHaveBeenCalled(); | ||
| expect(debugErrorSpy).not.toHaveBeenCalled(); | ||
| }); | ||
| it('resolves to the fallback and logs when an async callback rejects', async () => { | ||
| const error = new Error('boom'); | ||
| const fallback = vi.fn(async () => 'fallback'); | ||
| const result = safeCallback('callback threw:', () => Promise.reject(error), fallback); | ||
| await expect(result).resolves.toBe('fallback'); | ||
| expect(fallback).toHaveBeenCalledWith(error); | ||
| expect(debugErrorSpy).toHaveBeenCalledWith('callback threw:', error); | ||
| }); | ||
| it('does not treat non-thenable objects as promises', () => { | ||
| const value = { then: 'not a function' }; | ||
| expect( | ||
| safeCallback( | ||
| 'callback threw:', | ||
| () => value, | ||
| () => ({ then: 'fallback' }), | ||
| ), | ||
| ).toBe(value); | ||
| }); | ||
| }); |
Uh oh!
There was an error while loading. Please reload this page.
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.
unrelated to this PR, but we can possibly remove this warning (safe some bytes), this has been this way for some time 🤔 or at least make it a debug.warn gated by the debug flag so you can shake it out?
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.
yeah i think we can remove it tbh. has been like this since v9 and the type doesn't allow it anyway.