From 50c0e30936eabc0cd8df01c32ac6a1ffb9cb9c4c Mon Sep 17 00:00:00 2001
From: "Lucas Perais (lpe)" <lpe@odoo.com>
Date: Tue, 16 Nov 2021 16:26:39 +0100
Subject: [PATCH] [FIX] component: error_handling on current component

Have a Child Compnent which has one component that succeeds and another
one that fails at its instanciation.
The Child component handles the Errors by rendering itself.

Before this commit, the error handling algorithm made impossible for the scheduler to finish.
This was because the current fiber was still counted as ongoing, when it was actually completed.

After this commit, this use case is handled correctly.
---
 src/component/component_node.ts               |   2 +-
 src/component/error_handling.ts               |   6 +-
 .../__snapshots__/error_handling.test.ts.snap | 140 ++++++++++++++++++
 tests/components/error_handling.test.ts       |  78 ++++++++++
 4 files changed, 222 insertions(+), 4 deletions(-)

diff --git a/src/component/component_node.ts b/src/component/component_node.ts
index d8fcbcc2..0ab5d7fc 100644
--- a/src/component/component_node.ts
+++ b/src/component/component_node.ts
@@ -140,7 +140,7 @@ export class ComponentNode<T extends typeof Component = any> implements VNode<Co
     if (fiber && !fiber.bdom && !fibersInError.has(fiber)) {
       return fiber.root.promise;
     }
-    if (!this.bdom && !this.fiber) {
+    if (!this.bdom && !fiber) {
       // should find a way to return the future mounting promise
       return;
     }
diff --git a/src/component/error_handling.ts b/src/component/error_handling.ts
index f182dda3..dca56dd9 100644
--- a/src/component/error_handling.ts
+++ b/src/component/error_handling.ts
@@ -4,7 +4,7 @@ import type { Fiber } from "./fibers";
 export const fibersInError: WeakMap<Fiber, Error> = new WeakMap();
 export const nodeErrorHandlers: WeakMap<ComponentNode, ((error: Error) => void)[]> = new WeakMap();
 
-function _handleError(node: ComponentNode | null, error: Error): boolean {
+function _handleError(node: ComponentNode | null, error: Error, isFirstRound = false): boolean {
   if (!node) {
     return false;
   }
@@ -15,7 +15,7 @@ function _handleError(node: ComponentNode | null, error: Error): boolean {
 
   const errorHandlers = nodeErrorHandlers.get(node);
   if (errorHandlers) {
-    if (fiber && !fiber.children.length) {
+    if (isFirstRound && fiber) {
       fiber.root.counter--;
     }
 
@@ -42,7 +42,7 @@ export function handleError(node: ComponentNode, error: Error) {
   const fiber = node.fiber!;
   fibersInError.set(fiber.root, error);
 
-  if (!_handleError(node, error)) {
+  if (!_handleError(node, error, true)) {
     try {
       node.app.destroy();
     } catch (e) {}
diff --git a/tests/components/__snapshots__/error_handling.test.ts.snap b/tests/components/__snapshots__/error_handling.test.ts.snap
index 877f2074..3ae1771a 100644
--- a/tests/components/__snapshots__/error_handling.test.ts.snap
+++ b/tests/components/__snapshots__/error_handling.test.ts.snap
@@ -166,6 +166,76 @@ exports[`can catch errors can catch an error in the constructor call of a compon
 }"
 `;
 
+exports[`can catch errors can catch an error in the constructor call of a component render function 2 1`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component } = bdom;
+  let { withDefault, getTemplate, prepareList, withKey, zero, call, callSlot, capture, isBoundary, shallowEqual, setContextValue, toNumber, safeOutput } = helpers;
+  
+  let block1 = createBlock(\`<div>classic</div>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    return block1();
+  }
+}"
+`;
+
+exports[`can catch errors can catch an error in the constructor call of a component render function 2 2`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component } = bdom;
+  let { withDefault, getTemplate, prepareList, withKey, zero, call, callSlot, capture, isBoundary, shallowEqual, setContextValue, toNumber, safeOutput } = helpers;
+  
+  let block1 = createBlock(\`<div>Some text</div>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    return block1();
+  }
+}"
+`;
+
+exports[`can catch errors can catch an error in the constructor call of a component render function 2 3`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component } = bdom;
+  let { withDefault, getTemplate, prepareList, withKey, zero, call, callSlot, capture, isBoundary, shallowEqual, setContextValue, toNumber, safeOutput } = helpers;
+  
+  let block1 = createBlock(\`<div><block-child-0/><block-child-1/><block-child-1/></div>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    let b2,b3;
+    if (ctx['state'].error) {
+      b2 = text(\`Error handled\`);
+    } else {
+      b3 = callSlot(ctx, node, key, 'default');
+    }
+    return block1([], [b2, b3]);
+  }
+}"
+`;
+
+exports[`can catch errors can catch an error in the constructor call of a component render function 2 4`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component } = bdom;
+  let { withDefault, getTemplate, prepareList, withKey, zero, call, callSlot, capture, isBoundary, shallowEqual, setContextValue, toNumber, safeOutput } = helpers;
+  let assign = Object.assign;
+  
+  let block1 = createBlock(\`<div><block-child-0/></div>\`);
+  
+  const slot2 = ctx => (node, key) => {
+    let b3 = component(\`ClassicCompoent\`, {}, key + \`__3\`, node, ctx);
+    let b4 = component(\`ErrorComponent\`, {}, key + \`__4\`, node, ctx);
+    return multi([b3, b4]);
+  }
+  
+  return function template(ctx, node, key = \\"\\") {
+    let b5 = assign(component(\`ErrorBoundary\`, {}, key + \`__1\`, node, ctx), {slots: {'default': slot2(ctx)}});
+    return block1([], [b5]);
+  }
+}"
+`;
+
 exports[`can catch errors can catch an error in the constructor call of a component render function 3`] = `
 "function anonymous(bdom, helpers
 ) {
@@ -353,6 +423,76 @@ exports[`can catch errors can catch an error in the willStart call 3`] = `
 }"
 `;
 
+exports[`can catch errors can catch an error origination from a child's willStart function 1`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component } = bdom;
+  let { withDefault, getTemplate, prepareList, withKey, zero, call, callSlot, capture, isBoundary, shallowEqual, setContextValue, toNumber, safeOutput } = helpers;
+  
+  let block1 = createBlock(\`<div>classic</div>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    return block1();
+  }
+}"
+`;
+
+exports[`can catch errors can catch an error origination from a child's willStart function 2`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component } = bdom;
+  let { withDefault, getTemplate, prepareList, withKey, zero, call, callSlot, capture, isBoundary, shallowEqual, setContextValue, toNumber, safeOutput } = helpers;
+  
+  let block1 = createBlock(\`<div>Some text</div>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    return block1();
+  }
+}"
+`;
+
+exports[`can catch errors can catch an error origination from a child's willStart function 3`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component } = bdom;
+  let { withDefault, getTemplate, prepareList, withKey, zero, call, callSlot, capture, isBoundary, shallowEqual, setContextValue, toNumber, safeOutput } = helpers;
+  
+  let block1 = createBlock(\`<div><block-child-0/><block-child-1/><block-child-1/></div>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    let b2,b3;
+    if (ctx['state'].error) {
+      b2 = text(\`Error handled\`);
+    } else {
+      b3 = callSlot(ctx, node, key, 'default');
+    }
+    return block1([], [b2, b3]);
+  }
+}"
+`;
+
+exports[`can catch errors can catch an error origination from a child's willStart function 4`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component } = bdom;
+  let { withDefault, getTemplate, prepareList, withKey, zero, call, callSlot, capture, isBoundary, shallowEqual, setContextValue, toNumber, safeOutput } = helpers;
+  let assign = Object.assign;
+  
+  let block1 = createBlock(\`<div><block-child-0/></div>\`);
+  
+  const slot2 = ctx => (node, key) => {
+    let b3 = component(\`ClassicCompoent\`, {}, key + \`__3\`, node, ctx);
+    let b4 = component(\`ErrorComponent\`, {}, key + \`__4\`, node, ctx);
+    return multi([b3, b4]);
+  }
+  
+  return function template(ctx, node, key = \\"\\") {
+    let b5 = assign(component(\`ErrorBoundary\`, {}, key + \`__1\`, node, ctx), {slots: {'default': slot2(ctx)}});
+    return block1([], [b5]);
+  }
+}"
+`;
+
 exports[`can catch errors catchError in catchError 1`] = `
 "function anonymous(bdom, helpers
 ) {
diff --git a/tests/components/error_handling.test.ts b/tests/components/error_handling.test.ts
index f42da78c..edef5ffc 100644
--- a/tests/components/error_handling.test.ts
+++ b/tests/components/error_handling.test.ts
@@ -450,6 +450,44 @@ describe("can catch errors", () => {
     console.error = consoleError;
   });
 
+  test("can catch an error in the constructor call of a component render function 2", async () => {
+    const consoleError = console.error;
+    console.error = jest.fn();
+
+    class ClassicCompoent extends Component {
+      static template = xml`<div>classic</div>`;
+    }
+
+    class ErrorComponent extends Component {
+      static template = xml`<div>Some text</div>`;
+      setup() {
+        throw new Error("NOOOOO");
+      }
+    }
+    class ErrorBoundary extends Component {
+      static template = xml`<div>
+              <t t-if="state.error">Error handled</t>
+              <t t-else=""><t t-slot="default" /></t>
+          </div>`;
+      state = useState({ error: false });
+
+      setup() {
+        onError(() => (this.state.error = true));
+      }
+    }
+    class App extends Component {
+      static template = xml`<div>
+              <ErrorBoundary><ClassicCompoent/><ErrorComponent /></ErrorBoundary>
+          </div>`;
+      static components = { ErrorBoundary, ErrorComponent, ClassicCompoent };
+    }
+    await mount(App, fixture);
+    expect(fixture.innerHTML).toBe("<div><div>Error handled</div></div>");
+
+    expect(console.error).toBeCalledTimes(0);
+    console.error = consoleError;
+  });
+
   test("can catch an error in the willStart call", async () => {
     const consoleError = console.error;
     console.error = jest.fn();
@@ -486,6 +524,46 @@ describe("can catch errors", () => {
     console.error = consoleError;
   });
 
+  test("can catch an error origination from a child's willStart function", async () => {
+    const consoleError = console.error;
+    console.error = jest.fn();
+
+    class ClassicCompoent extends Component {
+      static template = xml`<div>classic</div>`;
+    }
+
+    class ErrorComponent extends Component {
+      static template = xml`<div>Some text</div>`;
+      setup() {
+        onWillStart(() => {
+          throw new Error("NOOOOO");
+        });
+      }
+    }
+    class ErrorBoundary extends Component {
+      static template = xml`<div>
+              <t t-if="state.error">Error handled</t>
+              <t t-else=""><t t-slot="default" /></t>
+          </div>`;
+      state = useState({ error: false });
+
+      setup() {
+        onError(() => (this.state.error = true));
+      }
+    }
+    class App extends Component {
+      static template = xml`<div>
+              <ErrorBoundary><ClassicCompoent/><ErrorComponent /></ErrorBoundary>
+          </div>`;
+      static components = { ErrorBoundary, ErrorComponent, ClassicCompoent };
+    }
+    await mount(App, fixture);
+    expect(fixture.innerHTML).toBe("<div><div>Error handled</div></div>");
+
+    expect(console.error).toBeCalledTimes(0);
+    console.error = consoleError;
+  });
+
   test.skip("can catch an error in the mounted call", async () => {
     // we do not catch error in mounted anymore
     console.error = jest.fn();