From 1da3ecdbee57949907a91f07cdc629e76ec72f5b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= Date: Tue, 30 Nov 2021 08:14:01 +0100 Subject: [PATCH] [FIX] component: improve error handling In the following situation: A parent of B, B parent of C, with an error when C is mounted, caught by B and retriggering a rendering in B, then the onMounted hook of A wasn't properly called. This commit fixes this problem. --- src/component/component_node.ts | 22 +++ src/component/error_handling.ts | 8 + src/component/fibers.ts | 24 ++- .../__snapshots__/error_handling.test.ts.snap | 95 +++++++++++ tests/components/error_handling.test.ts | 154 +++++++++++++++++- 5 files changed, 290 insertions(+), 13 deletions(-) diff --git a/src/component/component_node.ts b/src/component/component_node.ts index 58ced6d5..ae9522e0 100644 --- a/src/component/component_node.ts +++ b/src/component/component_node.ts @@ -221,6 +221,28 @@ export class ComponentNode this._render(fiber); } + /** + * Finds a child that has dom that is not yet updated, and update it. This + * method is meant to be used only in the context of repatching the dom after + * a mounted hook failed and was handled. + */ + updateDom() { + if (this.bdom === this.fiber!.bdom) { + // If the error was handled by some child component, we need to find it to + // apply its change + for (let k in this.children) { + const child = this.children[k]; + child.updateDom(); + } + } else { + // if we get here, this is the component that handled the error and rerendered + // itself, so we can simply patch the dom + this.bdom!.patch(this.fiber!.bdom, false); + this.fiber!.appliedToDom = true; + this.fiber = null; + } + } + // --------------------------------------------------------------------------- // Block DOM methods // --------------------------------------------------------------------------- diff --git a/src/component/error_handling.ts b/src/component/error_handling.ts index 1894cfcb..534d63ba 100644 --- a/src/component/error_handling.ts +++ b/src/component/error_handling.ts @@ -45,6 +45,14 @@ export function handleError(params: ErrorParams) { const node = "node" in params ? params.node : params.fiber.node; const fiber = "fiber" in params ? params.fiber : node.fiber!; + // resets the fibers on components if possible. This is important so that + // new renderings can be properly included in the initial one, if any. + let current: Fiber | null = fiber; + do { + current.node.fiber = current; + current = current.parent; + } while (current); + fibersInError.set(fiber.root, error); const handled = _handleError(node, error, true); diff --git a/src/component/fibers.ts b/src/component/fibers.ts index 92d185ad..e1942155 100644 --- a/src/component/fibers.ts +++ b/src/component/fibers.ts @@ -1,8 +1,7 @@ -import type { BDom } from "../blockdom"; -import { mount } from "../blockdom"; +import { BDom, mount } from "../blockdom"; import type { ComponentNode } from "./component_node"; -import { STATUS } from "./status"; import { fibersInError, handleError } from "./error_handling"; +import { STATUS } from "./status"; export function makeChildFiber(node: ComponentNode, parent: Fiber): Fiber { let current = node.fiber; @@ -30,6 +29,7 @@ export function makeRootFiber(node: ComponentNode): Fiber { if (fibersInError.has(current)) { fibersInError.delete(current); fibersInError.delete(root); + current.appliedToDom = false; } return current; } @@ -168,12 +168,20 @@ export class MountFiber extends RootFiber { let current: Fiber | undefined = this; try { const node = this.node; - node.bdom = this.bdom; - if (this.position === "last-child" || this.target.childNodes.length === 0) { - mount(node.bdom!, this.target); + if (node.bdom) { + // this is a complicated situation: if we mount a fiber with an existing + // bdom, this means that this same fiber was already completed, mounted, + // but a crash occurred in some mounted hook. Then, it was handled and + // the new rendering is being applied. + node.updateDom(); } else { - const firstChild = this.target.childNodes[0]; - mount(node.bdom!, this.target, firstChild); + node.bdom = this.bdom; + if (this.position === "last-child" || this.target.childNodes.length === 0) { + mount(node.bdom!, this.target); + } else { + const firstChild = this.target.childNodes[0]; + mount(node.bdom!, this.target, firstChild); + } } node.status = STATUS.MOUNTED; this.appliedToDom = true; diff --git a/tests/components/__snapshots__/error_handling.test.ts.snap b/tests/components/__snapshots__/error_handling.test.ts.snap index 491665c0..74749c63 100644 --- a/tests/components/__snapshots__/error_handling.test.ts.snap +++ b/tests/components/__snapshots__/error_handling.test.ts.snap @@ -364,6 +364,101 @@ exports[`can catch errors can catch an error in the initial call of a component }" `; +exports[`can catch errors can catch an error in the mounted call (in child of child) 1`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component, comment } = bdom; + let { withDefault, getTemplate, prepareList, withKey, zero, call, callSlot, capture, isBoundary, shallowEqual, setContextValue, toNumber, safeOutput } = helpers; + + let block1 = createBlock(\`
Some text
\`); + + return function template(ctx, node, key = \\"\\") { + return block1(); + } +}" +`; + +exports[`can catch errors can catch an error in the mounted call (in child of child) 2`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component, comment } = bdom; + let { withDefault, getTemplate, prepareList, withKey, zero, call, callSlot, capture, isBoundary, shallowEqual, setContextValue, toNumber, safeOutput } = helpers; + + let block1 = createBlock(\`
\`); + + return function template(ctx, node, key = \\"\\") { + let b2,b3; + if (ctx['state'].error) { + b2 = text(\`Error handled\`); + } else { + b3 = component(\`Boom\`, {}, key + \`__1\`, node, ctx); + } + return block1([], [b2, b3]); + } +}" +`; + +exports[`can catch errors can catch an error in the mounted call (in child of child) 3`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component, comment } = bdom; + let { withDefault, getTemplate, prepareList, withKey, zero, call, callSlot, capture, isBoundary, shallowEqual, setContextValue, toNumber, safeOutput } = helpers; + + let block1 = createBlock(\`
\`); + + return function template(ctx, node, key = \\"\\") { + let b2 = component(\`C\`, {}, key + \`__1\`, node, ctx); + return block1([], [b2]); + } +}" +`; + +exports[`can catch errors can catch an error in the mounted call (in child of child) 4`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component, comment } = bdom; + let { withDefault, getTemplate, prepareList, withKey, zero, call, callSlot, capture, isBoundary, shallowEqual, setContextValue, toNumber, safeOutput } = helpers; + + return function template(ctx, node, key = \\"\\") { + return component(\`B\`, {}, key + \`__1\`, node, ctx); + } +}" +`; + +exports[`can catch errors can catch an error in the mounted call (in root component) 1`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component, comment } = bdom; + let { withDefault, getTemplate, prepareList, withKey, zero, call, callSlot, capture, isBoundary, shallowEqual, setContextValue, toNumber, safeOutput } = helpers; + + let block1 = createBlock(\`
Some text
\`); + + return function template(ctx, node, key = \\"\\") { + return block1(); + } +}" +`; + +exports[`can catch errors can catch an error in the mounted call (in root component) 2`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component, comment } = bdom; + let { withDefault, getTemplate, prepareList, withKey, zero, call, callSlot, capture, isBoundary, shallowEqual, setContextValue, toNumber, safeOutput } = helpers; + + let block1 = createBlock(\`
\`); + + return function template(ctx, node, key = \\"\\") { + let b2,b3; + if (ctx['state'].error) { + b2 = text(\`Error handled\`); + } else { + b3 = component(\`ErrorComponent\`, {}, key + \`__1\`, node, ctx); + } + return block1([], [b2, b3]); + } +}" +`; + exports[`can catch errors can catch an error in the mounted call 1`] = ` "function anonymous(bdom, helpers ) { diff --git a/tests/components/error_handling.test.ts b/tests/components/error_handling.test.ts index 5840f838..82016f96 100644 --- a/tests/components/error_handling.test.ts +++ b/tests/components/error_handling.test.ts @@ -9,7 +9,13 @@ import { useState, } from "../../src/index"; import { xml } from "../../src/tags"; -import { makeTestFixture, nextTick, snapshotEverything } from "../helpers"; +import { + logStep, + makeTestFixture, + nextTick, + snapshotEverything, + useLogLifecycle, +} from "../helpers"; let fixture: HTMLElement; @@ -550,7 +556,9 @@ describe("can catch errors", () => { class ErrorComponent extends Component { static template = xml`
Some text
`; setup() { + useLogLifecycle(); onMounted(() => { + logStep("boom"); throw new Error("NOOOOO"); }); } @@ -563,20 +571,156 @@ describe("can catch errors", () => { state = useState({ error: false }); setup() { + useLogLifecycle(); onError(() => (this.state.error = true)); } } - class App extends Component { + class Root extends Component { static template = xml`
`; static components = { ErrorBoundary, ErrorComponent }; + setup() { + useLogLifecycle(); + } } - await mount(App, fixture); - await nextTick(); - await nextTick(); + await mount(Root, fixture); await nextTick(); expect(fixture.innerHTML).toBe("
Error handled
"); + expect([ + "Root:setup", + "Root:willStart", + "Root:willRender", + "ErrorBoundary:setup", + "ErrorBoundary:willStart", + "Root:rendered", + "ErrorBoundary:willRender", + "ErrorComponent:setup", + "ErrorComponent:willStart", + "ErrorBoundary:rendered", + "ErrorComponent:willRender", + "ErrorComponent:rendered", + "ErrorComponent:mounted", + "boom", + "ErrorBoundary:willRender", + "ErrorBoundary:rendered", + "ErrorBoundary:mounted", + "Root:mounted", + ]).toBeLogged(); + }); + + test("can catch an error in the mounted call (in root component)", async () => { + class ErrorComponent extends Component { + static template = xml`
Some text
`; + setup() { + useLogLifecycle(); + onMounted(() => { + logStep("boom"); + throw new Error("NOOOOO"); + }); + } + } + class Root extends Component { + static template = xml`
+ Error handled + +
`; + static components = { ErrorComponent }; + state = useState({ error: false }); + + setup() { + useLogLifecycle(); + onError(() => (this.state.error = true)); + } + } + await mount(Root, fixture); + await nextTick(); + expect(fixture.innerHTML).toBe("
Error handled
"); + expect([ + "Root:setup", + "Root:willStart", + "Root:willRender", + "ErrorComponent:setup", + "ErrorComponent:willStart", + "Root:rendered", + "ErrorComponent:willRender", + "ErrorComponent:rendered", + "ErrorComponent:mounted", + "boom", + "Root:willRender", + "Root:rendered", + "Root:mounted", + ]).toBeLogged(); + }); + + test("can catch an error in the mounted call (in child of child)", async () => { + class Boom extends Component { + static template = xml`
Some text
`; + setup() { + useLogLifecycle(); + onMounted(() => { + logStep("boom"); + throw new Error("NOOOOO"); + }); + } + } + + class C extends Component { + static template = xml`
+ Error handled + +
`; + static components = { Boom }; + state = useState({ error: false }); + + setup() { + useLogLifecycle(); + onError(() => (this.state.error = true)); + } + } + + class B extends Component { + static template = xml`
`; + static components = { C }; + setup() { + useLogLifecycle(); + } + } + class A extends Component { + static template = xml``; + static components = { B }; + setup() { + useLogLifecycle(); + } + } + await mount(A, fixture); + await nextTick(); + expect(fixture.innerHTML).toBe("
Error handled
"); + expect([ + "A:setup", + "A:willStart", + "A:willRender", + "B:setup", + "B:willStart", + "A:rendered", + "B:willRender", + "C:setup", + "C:willStart", + "B:rendered", + "C:willRender", + "Boom:setup", + "Boom:willStart", + "C:rendered", + "Boom:willRender", + "Boom:rendered", + "Boom:mounted", + "boom", + "C:willRender", + "C:rendered", + "C:mounted", + "B:mounted", + "A:mounted", + ]).toBeLogged(); }); test("can catch an error in the willPatch call", async () => {