From 67f86a4ab8815248658e580e230d116ffa2aaabb Mon Sep 17 00:00:00 2001
From: Samuel Degueldre <sad@odoo.com>
Date: Tue, 8 Mar 2022 09:37:39 +0100
Subject: [PATCH] [FIX] components: wrap onWillRender/onRendered hooks instead
 of renderFn

Previously, we were wrapping the entire renderFn in a try/catch, causing
errors during template execution to be caught and wrapped by
onWillRender/onRendered which is undesirable. Now we only wrap the hook
that's being registered.
---
 src/component/lifecycle_hooks.ts              | 14 +++---
 .../__snapshots__/error_handling.test.ts.snap | 13 ++++++
 tests/components/error_handling.test.ts       | 46 +++++++++++++++++++
 3 files changed, 67 insertions(+), 6 deletions(-)

diff --git a/src/component/lifecycle_hooks.ts b/src/component/lifecycle_hooks.ts
index 324077b5..3d7fafd5 100644
--- a/src/component/lifecycle_hooks.ts
+++ b/src/component/lifecycle_hooks.ts
@@ -77,21 +77,23 @@ export function onWillRender(fn: () => void | any) {
   const node = getCurrent();
   const renderFn = node.renderFn;
   const decorate = node.app.dev ? wrapError : (fn: any) => fn;
-  node.renderFn = decorate(() => {
-    fn.call(node.component);
+  fn = decorate(fn.bind(node.component), "onWillRender");
+  node.renderFn = () => {
+    fn();
     return renderFn();
-  }, "onWillRender");
+  };
 }
 
 export function onRendered(fn: () => void | any) {
   const node = getCurrent();
   const renderFn = node.renderFn;
   const decorate = node.app.dev ? wrapError : (fn: any) => fn;
-  node.renderFn = decorate(() => {
+  fn = decorate(fn.bind(node.component), "onRendered");
+  node.renderFn = () => {
     const result = renderFn();
-    fn.call(node.component);
+    fn();
     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 1f6bae4a..36f9bf47 100644
--- a/tests/components/__snapshots__/error_handling.test.ts.snap
+++ b/tests/components/__snapshots__/error_handling.test.ts.snap
@@ -1156,6 +1156,19 @@ exports[`errors and promises errors in mounted and in willUnmount 1`] = `
 }"
 `;
 
+exports[`errors and promises errors in onWillRender/onRender aren't wrapped more than once 1`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  
+  let block1 = createBlock(\`<div>abc</div>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    return block1();
+  }
+}"
+`;
+
 exports[`errors and promises errors in rerender 1`] = `
 "function anonymous(bdom, helpers
 ) {
diff --git a/tests/components/error_handling.test.ts b/tests/components/error_handling.test.ts
index 93bd42be..79a05363 100644
--- a/tests/components/error_handling.test.ts
+++ b/tests/components/error_handling.test.ts
@@ -5,6 +5,8 @@ import {
   onPatched,
   onWillPatch,
   onWillStart,
+  onWillRender,
+  onRendered,
   onWillUnmount,
   useState,
   xml,
@@ -199,6 +201,50 @@ describe("errors and promises", () => {
     expect(mockConsoleWarn).toBeCalledTimes(1);
   });
 
+  test("errors in onWillRender/onRender aren't wrapped more than once", async () => {
+    class App extends Component {
+      static template = xml`<div>abc</div>`;
+      setup() {
+        onWillRender(() => {
+          throw new Error("boom in onWillRender");
+        });
+        onRendered(() => {
+          throw new Error("boom in onRendered");
+        });
+      }
+    }
+
+    let error: Error;
+    try {
+      await mount(App, fixture, { test: true });
+    } catch (e) {
+      error = e as Error;
+    }
+    expect(error!).toBeDefined();
+    expect(error!.message).toBe(
+      `The following error occurred in onWillRender: "boom in onWillRender"`
+    );
+  });
+
+  test("error while rendering component isn't wrapped by onWillRender/onRendered", async () => {
+    class App extends Component {
+      static template = xml`<div t-att-class="{ 'invalid: 5 }">abc</div>`;
+      setup() {
+        onWillRender(() => {});
+        onRendered(() => {});
+      }
+    }
+
+    let error: Error;
+    try {
+      await mount(App, fixture, { test: true });
+    } catch (e) {
+      error = e as Error;
+    }
+    expect(error!).toBeDefined();
+    expect(error!.message).toBe("Tokenizer error: could not tokenize `{ 'invalid: 5 }`");
+  });
+
   test("an error in willPatch call will reject the render promise", async () => {
     class Root extends Component {
       static template = xml`<div><t t-esc="val"/></div>`;