From 04711c20246f7cdaac2305286fec783ab1859a18 Mon Sep 17 00:00:00 2001 From: Sigrid Huemer <32902192+s1gr1d@users.noreply.github.com> Date: Fri, 10 Jan 2025 11:37:06 +0100 Subject: [PATCH] fix(vue): Remove `logError` from `vueIntegration` (#14958) Removes `logError` and always re-throws the error when it's not handled by the user. PR for v8 (without removing `logError`): https://github.com/getsentry/sentry-javascript/pull/14943 Logging the error has been a problem in the past already (see https://github.com/getsentry/sentry-javascript/pull/7310). By just re-throwing the error we don't mess around with the initial message and stacktrace. --- .../vue-3/tests/errors.test.ts | 4 +- docs/migration/v8-to-v9.md | 6 + packages/vue/src/errorhandler.ts | 22 +- packages/vue/src/integration.ts | 1 - packages/vue/src/types.ts | 6 - packages/vue/test/errorHandler.test.ts | 209 +++++------------- 6 files changed, 71 insertions(+), 177 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/vue-3/tests/errors.test.ts b/dev-packages/e2e-tests/test-applications/vue-3/tests/errors.test.ts index 262cda11b366..b86e56eb4b83 100644 --- a/dev-packages/e2e-tests/test-applications/vue-3/tests/errors.test.ts +++ b/dev-packages/e2e-tests/test-applications/vue-3/tests/errors.test.ts @@ -19,7 +19,7 @@ test('sends an error', async ({ page }) => { type: 'Error', value: 'This is a Vue test error', mechanism: { - type: 'generic', + type: 'vue', handled: false, }, }, @@ -47,7 +47,7 @@ test('sends an error with a parameterized transaction name', async ({ page }) => type: 'Error', value: 'This is a Vue test error', mechanism: { - type: 'generic', + type: 'vue', handled: false, }, }, diff --git a/docs/migration/v8-to-v9.md b/docs/migration/v8-to-v9.md index 1f100c21d730..055c678e0d06 100644 --- a/docs/migration/v8-to-v9.md +++ b/docs/migration/v8-to-v9.md @@ -219,6 +219,9 @@ The following changes are unlikely to affect users of the SDK. They are listed h }); ``` +- The option `logErrors` in the `vueIntegration` has been removed. The Sentry Vue error handler will propagate the error to a user-defined error handler + or just re-throw the error (which will log the error without modifying). + ## 5. Build Changes Previously the CJS versions of the SDK code (wrongfully) contained compatibility statements for default exports in ESM: @@ -375,6 +378,9 @@ The Sentry metrics beta has ended and the metrics API has been removed from the }); ``` +- Deprecated `logErrors` in the `vueIntegration`. The Sentry Vue error handler will propagate the error to a user-defined error handler + or just re-throw the error (which will log the error without modifying). + ## `@sentry/nuxt` and `@sentry/vue` - When component tracking is enabled, "update" spans are no longer created by default. diff --git a/packages/vue/src/errorhandler.ts b/packages/vue/src/errorhandler.ts index c3dc88deb00f..f8ca3b9a2c9f 100644 --- a/packages/vue/src/errorhandler.ts +++ b/packages/vue/src/errorhandler.ts @@ -1,11 +1,11 @@ -import { captureException, consoleSandbox } from '@sentry/core'; +import { captureException } from '@sentry/core'; import type { ViewModel, Vue, VueOptions } from './types'; import { formatComponentName, generateComponentTrace } from './vendor/components'; type UnknownFunc = (...args: unknown[]) => void; export const attachErrorHandler = (app: Vue, options: VueOptions): void => { - const { errorHandler: originalErrorHandler, warnHandler, silent } = app.config; + const { errorHandler: originalErrorHandler } = app.config; app.config.errorHandler = (error: Error, vm: ViewModel, lifecycleHook: string): void => { const componentName = formatComponentName(vm, false); @@ -30,27 +30,15 @@ export const attachErrorHandler = (app: Vue, options: VueOptions): void => { setTimeout(() => { captureException(error, { captureContext: { contexts: { vue: metadata } }, - mechanism: { handled: false }, + mechanism: { handled: !!originalErrorHandler, type: 'vue' }, }); }); // Check if the current `app.config.errorHandler` is explicitly set by the user before calling it. if (typeof originalErrorHandler === 'function' && app.config.errorHandler) { (originalErrorHandler as UnknownFunc).call(app, error, vm, lifecycleHook); - } - - if (options.logErrors) { - const hasConsole = typeof console !== 'undefined'; - const message = `Error in ${lifecycleHook}: "${error?.toString()}"`; - - if (warnHandler) { - (warnHandler as UnknownFunc).call(null, message, vm, trace); - } else if (hasConsole && !silent) { - consoleSandbox(() => { - // eslint-disable-next-line no-console - console.error(`[Vue warn]: ${message}${trace}`); - }); - } + } else { + throw error; } }; }; diff --git a/packages/vue/src/integration.ts b/packages/vue/src/integration.ts index 93fe25aee3d9..167ea5c18d0c 100644 --- a/packages/vue/src/integration.ts +++ b/packages/vue/src/integration.ts @@ -10,7 +10,6 @@ const globalWithVue = GLOBAL_OBJ as typeof GLOBAL_OBJ & { Vue: Vue }; const DEFAULT_CONFIG: VueOptions = { Vue: globalWithVue.Vue, attachProps: true, - logErrors: true, attachErrorHandler: true, tracingOptions: { hooks: DEFAULT_HOOKS, diff --git a/packages/vue/src/types.ts b/packages/vue/src/types.ts index af0613c7fbe9..6f61fc4e6104 100644 --- a/packages/vue/src/types.ts +++ b/packages/vue/src/types.ts @@ -41,12 +41,6 @@ export interface VueOptions { */ attachProps: boolean; - /** - * When set to `true`, original Vue's `logError` will be called as well. - * https://github.com/vuejs/vue/blob/c2b1cfe9ccd08835f2d99f6ce60f67b4de55187f/src/core/util/error.js#L38-L48 - */ - logErrors: boolean; - /** * By default, Sentry attaches an error handler to capture exceptions and report them to Sentry. * When `attachErrorHandler` is set to `false`, automatic error reporting is disabled. diff --git a/packages/vue/test/errorHandler.test.ts b/packages/vue/test/errorHandler.test.ts index 273d1dfecd0e..4f44fc4275d3 100644 --- a/packages/vue/test/errorHandler.test.ts +++ b/packages/vue/test/errorHandler.test.ts @@ -1,32 +1,34 @@ -import { afterEach, describe, expect, test, vi } from 'vitest'; +import { afterEach, describe, expect, it, test, vi } from 'vitest'; import { setCurrentClient } from '@sentry/browser'; import { attachErrorHandler } from '../src/errorhandler'; import type { Operation, Options, ViewModel, Vue } from '../src/types'; -import { generateComponentTrace } from '../src/vendor/components'; describe('attachErrorHandler', () => { - describe('attachProps', () => { + describe('attach data to captureException', () => { afterEach(() => { vi.resetAllMocks(); + // we need timers to still call captureException wrapped inside setTimeout after the error throws + vi.useRealTimers(); }); describe("given I don't want to `attachProps`", () => { test('no `propsData` is added to the metadata', () => { - // arrange const t = testHarness({ - enableErrorHandler: false, enableWarnHandler: false, attachProps: false, vm: null, + enableConsole: true, }); - // act - t.run(); + vi.useFakeTimers(); + expect(() => t.run()).toThrow(DummyError); + vi.runAllTimers(); // assert t.expect.errorToHaveBeenCaptured().withoutProps(); + t.expect.errorToHaveBeenCaptured().withMechanismMetadata({ handled: false, type: 'vue' }); }); }); @@ -41,10 +43,13 @@ describe('attachErrorHandler', () => { }); // act - t.run(); + vi.useFakeTimers(); + expect(() => t.run()).toThrow(DummyError); + vi.runAllTimers(); // assert t.expect.errorToHaveBeenCaptured().withoutProps(); + t.expect.errorToHaveBeenCaptured().withMechanismMetadata({ handled: false, type: 'vue' }); }); }); @@ -58,7 +63,9 @@ describe('attachErrorHandler', () => { }); // act - t.run(); + vi.useFakeTimers(); + expect(() => t.run()).toThrow(DummyError); + vi.runAllTimers(); // assert t.expect.errorToHaveBeenCaptured().withoutProps(); @@ -76,7 +83,9 @@ describe('attachErrorHandler', () => { }); // act - t.run(); + vi.useFakeTimers(); + expect(() => t.run()).toThrow(DummyError); + vi.runAllTimers(); // assert t.expect.errorToHaveBeenCaptured().withoutProps(); @@ -94,7 +103,9 @@ describe('attachErrorHandler', () => { }); // act - t.run(); + vi.useFakeTimers(); + expect(() => t.run()).toThrow(DummyError); + vi.runAllTimers(); // assert t.expect.errorToHaveBeenCaptured().withProps(props); @@ -114,7 +125,9 @@ describe('attachErrorHandler', () => { }); // act - t.run(); + vi.useFakeTimers(); + expect(() => t.run()).toThrow(DummyError); + vi.runAllTimers(); // assert t.expect.errorToHaveBeenCaptured().withProps(props); @@ -123,31 +136,22 @@ describe('attachErrorHandler', () => { }); }); }); - }); - describe('provided errorHandler', () => { - describe('given I did not provide an `errorHandler`', () => { - test('it is not called', () => { + describe('attach mechanism metadata', () => { + it('should mark error as unhandled and capture correct metadata', () => { // arrange - const t = testHarness({ - enableErrorHandler: false, - vm: { - $options: { - name: 'stub-vm', - }, - }, - }); + const t = testHarness({ vm: null }); // act - t.run(); + vi.useFakeTimers(); + expect(() => t.run()).toThrow(DummyError); + vi.runAllTimers(); // assert - t.expect.errorHandlerSpy.not.toHaveBeenCalled(); + t.expect.errorToHaveBeenCaptured().withMechanismMetadata({ handled: false, type: 'vue' }); }); - }); - describe('given I provided an `errorHandler`', () => { - test('it is called', () => { + it('should mark error as handled and properly delegate to error handler', () => { // arrange const vm = { $options: { @@ -156,6 +160,7 @@ describe('attachErrorHandler', () => { }; const t = testHarness({ enableErrorHandler: true, + enableConsole: true, vm, }); @@ -164,137 +169,38 @@ describe('attachErrorHandler', () => { // assert t.expect.errorHandlerSpy.toHaveBeenCalledWith(expect.any(Error), vm, 'stub-lifecycle-hook'); + t.expect.errorToHaveBeenCaptured().withMechanismMetadata({ handled: true, type: 'vue' }); }); }); }); - describe('error logging', () => { - describe('given I disabled error logging', () => { - describe('when an error is captured', () => { - test('it logs nothing', () => { - // arrange - const vm = { - $options: { - name: 'stub-vm', - }, - }; - const t = testHarness({ - enableWarnHandler: false, - logErrors: false, - vm, - }); - - // act - t.run(); - - // assert - t.expect.consoleErrorSpy.not.toHaveBeenCalled(); - t.expect.warnHandlerSpy.not.toHaveBeenCalled(); - }); - }); + describe('error re-throwing and logging', () => { + afterEach(() => { + vi.resetAllMocks(); }); - describe('given I enabled error logging', () => { - describe('when I provide a `warnHandler`', () => { - describe('when a error is captured', () => { - test.each([ - [ - 'with wm', - { - $options: { - name: 'stub-vm', - }, - }, - generateComponentTrace({ - $options: { - name: 'stub-vm', - }, - } as ViewModel), - ], - ['without vm', null, ''], - ])('it calls my `warnHandler` (%s)', (_, vm, expectedTrace) => { - // arrange - const t = testHarness({ - vm, - logErrors: true, - enableWarnHandler: true, - }); - - // act - t.run(); - - // assert - t.expect.consoleErrorSpy.not.toHaveBeenCalled(); - t.expect.warnHandlerSpy.toHaveBeenCalledWith( - 'Error in stub-lifecycle-hook: "DummyError: just an error"', - vm, - expectedTrace, - ); - }); - }); - }); - - describe('when I do not provide a `warnHandler`', () => { - describe("and I don't have a console", () => { - test('it logs nothing', () => { - // arrange - const vm = { - $options: { - name: 'stub-vm', - }, - }; - const t = testHarness({ - vm, - logErrors: true, - enableConsole: false, - }); - - // act - t.run(); - - // assert - t.expect.consoleErrorSpy.not.toHaveBeenCalled(); - }); + describe('error re-throwing', () => { + it('should re-throw error when no error handler exists', () => { + const t = testHarness({ + enableErrorHandler: false, + enableConsole: true, + vm: { $options: { name: 'stub-vm' } }, }); - describe('and I silenced logging in Vue', () => { - test('it logs nothing', () => { - // arrange - const vm = { - $options: { - name: 'stub-vm', - }, - }; - const t = testHarness({ - vm, - logErrors: true, - silent: true, - }); - - // act - t.run(); + expect(() => t.run()).toThrow(DummyError); + }); - // assert - t.expect.consoleErrorSpy.not.toHaveBeenCalled(); - }); + it('should call user-defined error handler when provided', () => { + const vm = { $options: { name: 'stub-vm' } }; + const t = testHarness({ + enableErrorHandler: true, + enableConsole: true, + vm, }); - test('it call `console.error`', () => { - // arrange - const t = testHarness({ - vm: null, - logErrors: true, - enableConsole: true, - }); - - // act - t.run(); + t.run(); - // assert - t.expect.consoleErrorSpy.toHaveBeenCalledWith( - '[Vue warn]: Error in stub-lifecycle-hook: "DummyError: just an error"', - ); - }); + t.expect.errorHandlerSpy.toHaveBeenCalledWith(expect.any(Error), vm, 'stub-lifecycle-hook'); }); }); }); @@ -308,7 +214,6 @@ type TestHarnessOpts = { enableConsole?: boolean; silent?: boolean; attachProps?: boolean; - logErrors?: boolean; }; class DummyError extends Error { @@ -321,7 +226,6 @@ class DummyError extends Error { const testHarness = ({ silent, attachProps, - logErrors, enableWarnHandler, enableErrorHandler, enableConsole, @@ -366,7 +270,6 @@ const testHarness = ({ const options: Options = { attachProps: !!attachProps, - logErrors: !!logErrors, tracingOptions: {}, trackComponents: [], timeout: 0, @@ -393,6 +296,7 @@ const testHarness = ({ expect(captureExceptionSpy).toHaveBeenCalledTimes(1); const error = captureExceptionSpy.mock.calls[0][0]; const contexts = captureExceptionSpy.mock.calls[0][1]?.captureContext.contexts; + const mechanismMetadata = captureExceptionSpy.mock.calls[0][1]?.mechanism; expect(error).toBeInstanceOf(DummyError); @@ -403,6 +307,9 @@ const testHarness = ({ withoutProps: () => { expect(contexts).not.toHaveProperty('vue.propsData'); }, + withMechanismMetadata: (mechanism: { handled: boolean; type: 'vue' }) => { + expect(mechanismMetadata).toEqual(mechanism); + }, }; }, },