From 804ad3c35ef3b932af8e9a2479ba0ff81d44d52a Mon Sep 17 00:00:00 2001 From: Samuel Degueldre Date: Thu, 10 Feb 2022 14:47:16 +0100 Subject: [PATCH] [IMP] component: improve errors when thrown from lifecycle hooks Previously, a crash in a lifecycle hook for any reason would throw an error whose stack trace started from the scheduler and contained only the place where the hook was called by owl, but not the place where the hook was registered by the user. This proved very difficult for users to debug as they cannot really tell which component registered that hook. This commit alleviates the issue by creating a new Error when the hook is originally called, and wrapping the registered callback in a try catch, throwing an error with the correct stack trace instead of the error in the hook, and setting the error in the hook as the cause of this synthetic error. --- src/component/lifecycle_hooks.ts | 57 +++++++++++++++---- .../__snapshots__/error_handling.test.ts.snap | 13 +++++ tests/components/error_handling.test.ts | 55 ++++++++++++------ 3 files changed, 97 insertions(+), 28 deletions(-) diff --git a/src/component/lifecycle_hooks.ts b/src/component/lifecycle_hooks.ts index 654266b6..324077b5 100644 --- a/src/component/lifecycle_hooks.ts +++ b/src/component/lifecycle_hooks.ts @@ -1,62 +1,97 @@ import { getCurrent } from "./component_node"; import { nodeErrorHandlers } from "./error_handling"; +function wrapError(fn: (...args: any[]) => any, hookName: string) { + const error = new Error(`The following error occurred in ${hookName}: `) as Error & { + cause: any; + }; + return (...args: any[]) => { + try { + const result = fn(...args); + if (result instanceof Promise) { + return result.catch((cause) => { + error.cause = cause; + if (cause instanceof Error) { + error.message += `"${cause.message}"`; + } + throw error; + }); + } + return result; + } catch (cause) { + if (cause instanceof Error) { + error.message += `"${cause.message}"`; + } + throw error; + } + }; +} + // ----------------------------------------------------------------------------- // hooks // ----------------------------------------------------------------------------- export function onWillStart(fn: () => Promise | void | any) { const node = getCurrent(); - node.willStart.push(fn.bind(node.component)); + const decorate = node.app.dev ? wrapError : (fn: any) => fn; + node.willStart.push(decorate(fn.bind(node.component), "onWillStart")); } export function onWillUpdateProps(fn: (nextProps: any) => Promise | void | any) { const node = getCurrent(); - node.willUpdateProps.push(fn.bind(node.component)); + const decorate = node.app.dev ? wrapError : (fn: any) => fn; + node.willUpdateProps.push(decorate(fn.bind(node.component), "onWillUpdateProps")); } export function onMounted(fn: () => void | any) { const node = getCurrent(); - node.mounted.push(fn.bind(node.component)); + const decorate = node.app.dev ? wrapError : (fn: any) => fn; + node.mounted.push(decorate(fn.bind(node.component), "onMounted")); } export function onWillPatch(fn: () => Promise | any | void) { const node = getCurrent(); - node.willPatch.unshift(fn.bind(node.component)); + const decorate = node.app.dev ? wrapError : (fn: any) => fn; + node.willPatch.unshift(decorate(fn.bind(node.component), "onWillPatch")); } export function onPatched(fn: () => void | any) { const node = getCurrent(); - node.patched.push(fn.bind(node.component)); + const decorate = node.app.dev ? wrapError : (fn: any) => fn; + node.patched.push(decorate(fn.bind(node.component), "onPatched")); } export function onWillUnmount(fn: () => Promise | void | any) { const node = getCurrent(); - node.willUnmount.unshift(fn.bind(node.component)); + const decorate = node.app.dev ? wrapError : (fn: any) => fn; + node.willUnmount.unshift(decorate(fn.bind(node.component), "onWillUnmount")); } export function onWillDestroy(fn: () => Promise | void | any) { const node = getCurrent(); - node.willDestroy.push(fn.bind(node.component)); + const decorate = node.app.dev ? wrapError : (fn: any) => fn; + node.willDestroy.push(decorate(fn.bind(node.component), "onWillDestroy")); } export function onWillRender(fn: () => void | any) { const node = getCurrent(); const renderFn = node.renderFn; - node.renderFn = () => { + const decorate = node.app.dev ? wrapError : (fn: any) => fn; + node.renderFn = decorate(() => { fn.call(node.component); return renderFn(); - }; + }, "onWillRender"); } export function onRendered(fn: () => void | any) { const node = getCurrent(); const renderFn = node.renderFn; - node.renderFn = () => { + const decorate = node.app.dev ? wrapError : (fn: any) => fn; + node.renderFn = decorate(() => { const result = renderFn(); fn.call(node.component); return result; - }; + }, "onRendered"); } type OnErrorCallback = (error: any) => void | any; diff --git a/tests/components/__snapshots__/error_handling.test.ts.snap b/tests/components/__snapshots__/error_handling.test.ts.snap index cc50f71b..c393dce3 100644 --- a/tests/components/__snapshots__/error_handling.test.ts.snap +++ b/tests/components/__snapshots__/error_handling.test.ts.snap @@ -1092,6 +1092,19 @@ exports[`errors and promises an error in mounted call will reject the mount prom }" `; +exports[`errors and promises an error in onMounted callback will have the component's setup in its stack trace 1`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component, comment } = bdom; + + let block1 = createBlock(\`
abc
\`); + + return function template(ctx, node, key = \\"\\") { + return block1(); + } +}" +`; + exports[`errors and promises an error in patched call will reject the render promise 1`] = ` "function anonymous(bdom, helpers ) { diff --git a/tests/components/error_handling.test.ts b/tests/components/error_handling.test.ts index 078a96a0..93bd42be 100644 --- a/tests/components/error_handling.test.ts +++ b/tests/components/error_handling.test.ts @@ -82,8 +82,6 @@ describe("basics", () => { }); test("display a nice error if it cannot find component (in dev mode)", async () => { - const info = console.info; - console.info = jest.fn(() => {}); // dev mode message class SomeComponent extends Component {} class Parent extends Component { static template = xml``; @@ -91,7 +89,7 @@ describe("basics", () => { } let error: Error; try { - await mount(Parent, fixture, { dev: true }); + await mount(Parent, fixture, { test: true }); } catch (e) { error = e as Error; } @@ -100,8 +98,6 @@ describe("basics", () => { expect(console.error).toBeCalledTimes(0); expect(mockConsoleError).toBeCalledTimes(0); expect(mockConsoleWarn).toBeCalledTimes(1); - expect(console.info).toBeCalledTimes(1); - console.info = info; }); test("simple catchError", async () => { @@ -179,6 +175,30 @@ describe("errors and promises", () => { expect(mockConsoleWarn).toBeCalledTimes(1); }); + test("an error in onMounted callback will have the component's setup in its stack trace", async () => { + class App extends Component { + static template = xml`
abc
`; + setup() { + onMounted(() => { + throw new Error("boom"); + }); + } + } + + let error: Error; + try { + await mount(App, fixture, { test: true }); + } catch (e) { + error = e as Error; + } + expect(error!).toBeDefined(); + expect(error!.stack).toContain("App.setup"); + expect(error!.stack).toContain("error_handling.test.ts"); + expect(fixture.innerHTML).toBe(""); + expect(mockConsoleError).toBeCalledTimes(0); + expect(mockConsoleWarn).toBeCalledTimes(1); + }); + test("an error in willPatch call will reject the render promise", async () => { class Root extends Component { static template = xml`
`; @@ -191,13 +211,13 @@ describe("errors and promises", () => { } } - const root = await mount(Root, fixture); + const root = await mount(Root, fixture, { test: true }); root.val = 4; let error: Error; root.render(); await nextTick(); expect(error!).toBeDefined(); - expect(error!.message).toBe("boom"); + expect(error!.message).toBe(`The following error occurred in onWillPatch: "boom"`); expect(mockConsoleError).toBeCalledTimes(0); expect(mockConsoleWarn).toBeCalledTimes(0); }); @@ -214,13 +234,13 @@ describe("errors and promises", () => { } } - const root = await mount(Root, fixture); + const root = await mount(Root, fixture, { test: true }); root.val = 4; let error: Error; root.render(); await nextTick(); expect(error!).toBeDefined(); - expect(error!.message).toBe("boom"); + expect(error!.message).toBe(`The following error occurred in onPatched: "boom"`); expect(mockConsoleError).toBeCalledTimes(0); expect(mockConsoleWarn).toBeCalledTimes(0); }); @@ -296,7 +316,6 @@ describe("errors and promises", () => { }); test("errors in mounted and in willUnmount", async () => { - expect.assertions(4); class Example extends Component { static template = xml`
`; val: any; @@ -313,9 +332,11 @@ describe("errors and promises", () => { } try { - await mount(Example, fixture); + await mount(Example, fixture, { test: true }); } catch (e) { - expect((e as Error).message).toBe("Error in mounted"); + expect((e as Error).message).toBe( + `The following error occurred in onMounted: "Error in mounted"` + ); } // 1 additional error is logged because the destruction of the app causes // the onWillUnmount hook to be called and to fail @@ -385,14 +406,14 @@ describe("can catch errors", () => { }); } } - let e: any = null; + let e: Error; try { - await mount(Root, fixture); + await mount(Root, fixture, { test: true }); } catch (error) { - e = error; + e = error as Error; } - expect(e.message).toBe( - "No active component (a hook function should only be called in 'setup')" + expect(e!.message).toBe( + `The following error occurred in onWillStart: "No active component (a hook function should only be called in 'setup')"` ); });