From 7fb166bd5065296c070d4dff13257a04746217f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= Date: Thu, 31 Mar 2022 10:35:09 +0200 Subject: [PATCH] [FIX] component: protect against errors in onWillDestroy --- src/component/component_node.ts | 18 ++- src/component/error_handling.ts | 14 +- src/component/fibers.ts | 3 +- .../__snapshots__/error_handling.test.ts.snap | 58 ++++++++ tests/components/concurrency.test.ts | 5 + tests/components/error_handling.test.ts | 124 +++++++++++++++++- 6 files changed, 204 insertions(+), 18 deletions(-) diff --git a/src/component/component_node.ts b/src/component/component_node.ts index 656d6727..2e1b4ad9 100644 --- a/src/component/component_node.ts +++ b/src/component/component_node.ts @@ -80,10 +80,7 @@ export function component

( let isDynamic = typeof name !== "string"; if (node) { - if (node.status < STATUS.MOUNTED) { - node.destroy(); - node = undefined; - } else if (node.status === STATUS.DESTROYED) { + if (node.status === STATUS.DESTROYED) { node = undefined; } } @@ -194,7 +191,7 @@ export class ComponentNode

implements VNode implements VNode implements VNode = new WeakMap(); export const nodeErrorHandlers: WeakMap void)[]> = new WeakMap(); -function _handleError(node: ComponentNode | null, error: any, isFirstRound = false): boolean { +function _handleError(node: ComponentNode | null, error: any): boolean { if (!node) { return false; } @@ -16,23 +16,19 @@ function _handleError(node: ComponentNode | null, error: any, isFirstRound = fal const errorHandlers = nodeErrorHandlers.get(node); if (errorHandlers) { - let stopped = false; + let handled = false; // execute in the opposite order for (let i = errorHandlers.length - 1; i >= 0; i--) { try { errorHandlers[i](error); - stopped = true; + handled = true; break; } catch (e) { error = e; } } - if (stopped) { - if (isFirstRound && fiber && fiber.node.fiber) { - const root = fiber.root!; - root.setCounter(root.counter - 1); - } + if (handled) { return true; } } @@ -55,7 +51,7 @@ export function handleError(params: ErrorParams) { fibersInError.set(fiber.root!, error); - const handled = _handleError(node, error, true); + const handled = _handleError(node, error); if (!handled) { console.warn(`[Owl] Unhandled error. Destroying the root component`); try { diff --git a/src/component/fibers.ts b/src/component/fibers.ts index 169e02dd..e65082d5 100644 --- a/src/component/fibers.ts +++ b/src/component/fibers.ts @@ -124,11 +124,12 @@ export class Fiber { const root = this.root; if (root) { try { + (this.bdom as any) = true; this.bdom = node.renderFn(); - root.setCounter(root.counter - 1); } catch (e) { handleError({ node, error: e }); } + root.setCounter(root.counter - 1); } } } diff --git a/tests/components/__snapshots__/error_handling.test.ts.snap b/tests/components/__snapshots__/error_handling.test.ts.snap index 0d92228e..1c96ad70 100644 --- a/tests/components/__snapshots__/error_handling.test.ts.snap +++ b/tests/components/__snapshots__/error_handling.test.ts.snap @@ -85,6 +85,64 @@ exports[`basics simple catchError 2`] = ` }" `; +exports[`can catch errors an error in onWillDestroy 1`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component, comment } = bdom; + + return function template(ctx, node, key = \\"\\") { + let b2,b3; + b2 = text(ctx['state'].value); + if (ctx['state'].hasChild) { + b3 = component(\`Child\`, {}, key + \`__1\`, node, ctx); + } + return multi([b2, b3]); + } +}" +`; + +exports[`can catch errors an error in onWillDestroy 2`] = ` +"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[`can catch errors an error in onWillDestroy, variation 1`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component, comment } = bdom; + + return function template(ctx, node, key = \\"\\") { + let b2,b3; + b2 = text(ctx['state'].value); + if (ctx['state'].hasChild) { + b3 = component(\`Child\`, {}, key + \`__1\`, node, ctx); + } + return multi([b2, b3]); + } +}" +`; + +exports[`can catch errors an error in onWillDestroy, variation 2`] = ` +"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[`can catch errors calling a hook outside setup should crash 1`] = ` "function anonymous(bdom, helpers ) { diff --git a/tests/components/concurrency.test.ts b/tests/components/concurrency.test.ts index dbf2b3c4..48349d1b 100644 --- a/tests/components/concurrency.test.ts +++ b/tests/components/concurrency.test.ts @@ -172,6 +172,11 @@ test("destroying/recreating a subcomponent, other scenario", async () => { await nextTick(); expect([ + "Parent:willRender", + "Child:setup", + "Child:willStart", + "Parent:rendered", + "Child:willDestroy", "Parent:willRender", "Child:setup", "Child:willStart", diff --git a/tests/components/error_handling.test.ts b/tests/components/error_handling.test.ts index 79a05363..659b099d 100644 --- a/tests/components/error_handling.test.ts +++ b/tests/components/error_handling.test.ts @@ -1,4 +1,4 @@ -import { Component, mount } from "../../src"; +import { Component, mount, onWillDestroy } from "../../src"; import { onError, onMounted, @@ -1197,4 +1197,126 @@ describe("can catch errors", () => { expect(fixture.innerHTML).toBe("
Child 2
"); expect(steps).toEqual(["Error Component"]); }); + + test("an error in onWillDestroy", async () => { + class Child extends Component { + static template = xml`
abc
`; + setup() { + useLogLifecycle(); + onWillDestroy(() => { + throw new Error("boom"); + }); + } + } + + class Parent extends Component { + static template = xml` + + `; + static components = { Child }; + + state = useState({ value: 1, hasChild: true }); + setup() { + useLogLifecycle(); + onError(() => { + this.state.value++; + }); + } + } + + const parent = await mount(Parent, fixture); + expect(fixture.innerHTML).toBe("1
abc
"); + expect([ + "Parent:setup", + "Parent:willStart", + "Parent:willRender", + "Child:setup", + "Child:willStart", + "Parent:rendered", + "Child:willRender", + "Child:rendered", + "Child:mounted", + "Parent:mounted", + ]).toBeLogged(); + parent.state.hasChild = false; + await nextTick(); + await nextTick(); + await nextTick(); + await nextTick(); + expect([ + "Parent:willRender", + "Parent:rendered", + "Parent:willPatch", + "Child:willUnmount", + "Child:willDestroy", + "Parent:willRender", + "Parent:rendered", + "Parent:willPatch", + "Parent:patched", + ]).toBeLogged(); + expect(fixture.innerHTML).toBe("2"); + }); + + test("an error in onWillDestroy, variation", async () => { + class Child extends Component { + static template = xml`
abc
`; + setup() { + useLogLifecycle(); + onWillDestroy(() => { + throw new Error("boom"); + }); + } + } + + class Parent extends Component { + static template = xml` + + `; + static components = { Child }; + + state = useState({ value: 1, hasChild: false }); + setup() { + useLogLifecycle(); + onError(() => { + this.state.value++; + }); + } + } + + const parent = await mount(Parent, fixture); + expect(fixture.innerHTML).toBe("1"); + + expect([ + "Parent:setup", + "Parent:willStart", + "Parent:willRender", + "Parent:rendered", + "Parent:mounted", + ]).toBeLogged(); + + parent.state.hasChild = true; + await nextMicroTick(); + await nextMicroTick(); + await nextMicroTick(); + await nextMicroTick(); + await nextMicroTick(); + expect([ + "Parent:willRender", + "Child:setup", + "Child:willStart", + "Parent:rendered", + "Child:willRender", + "Child:rendered", + ]).toBeLogged(); + parent.state.hasChild = false; + await nextTick(); + expect([ + "Child:willDestroy", + "Parent:willRender", + "Parent:rendered", + "Parent:willPatch", + "Parent:patched", + ]).toBeLogged(); + expect(fixture.innerHTML).toBe("2"); + }); });