From 9e37b968e83dc93fed5c2c2c88cc8549dbc2e878 Mon Sep 17 00:00:00 2001
From: Aaron Bohy <aab@odoo.com>
Date: Wed, 30 Oct 2019 11:35:28 +0100
Subject: [PATCH] [FIX] component: error handling: rendering with sub
 components

Closes #425
---
 src/component/component.ts        | 20 +++++++++-----------
 src/component/fiber.ts            |  5 +++++
 src/component/scheduler.ts        |  3 +++
 src/context.ts                    |  3 ++-
 tests/component/component.test.ts | 20 ++++++++++++++++++++
 tests/helpers.ts                  |  3 ++-
 6 files changed, 41 insertions(+), 13 deletions(-)

diff --git a/src/component/component.ts b/src/component/component.ts
index be85afb2..b283edf7 100644
--- a/src/component/component.ts
+++ b/src/component/component.ts
@@ -4,7 +4,7 @@ import { h, patch, VNode } from "../vdom/index";
 import "./directive";
 import { Fiber } from "./fiber";
 import "./props_validation";
-import { Scheduler } from "./scheduler";
+import { scheduler } from "./scheduler";
 
 /**
  * Owl Component System
@@ -20,8 +20,6 @@ import { Scheduler } from "./scheduler";
 //------------------------------------------------------------------------------
 // Types/helpers
 //------------------------------------------------------------------------------
-const raf = window.requestAnimationFrame.bind(window);
-export const scheduler = new Scheduler(raf);
 
 /**
  * An Env (environment) is an object that will be (mostly) shared between all
@@ -296,13 +294,8 @@ export class Component<T extends Env, Props extends {}> {
       }
       return;
     }
-    const fiber = new Fiber(null, this, this.props, undefined, undefined, false);
-    if (!__owl__.vnode) {
-      this.__prepareAndRender(fiber);
-    } else {
-      this.__render(fiber);
-    }
     return new Promise((resolve, reject) => {
+      const fiber = new Fiber(null, this, this.props, undefined, undefined, false);
       scheduler.addFiber(fiber, err => {
         if (err) {
           reject(err);
@@ -317,6 +310,11 @@ export class Component<T extends Env, Props extends {}> {
         }
         resolve();
       });
+      if (!__owl__.vnode) {
+        this.__prepareAndRender(fiber);
+      } else {
+        this.__render(fiber);
+      }
     });
   }
 
@@ -348,9 +346,8 @@ export class Component<T extends Env, Props extends {}> {
     ) {
       return;
     }
-    const fiber = new Fiber(null, this, this.props, undefined, undefined, force);
-    this.__render(fiber);
     return new Promise((resolve, reject) => {
+      const fiber = new Fiber(null, this, this.props, undefined, undefined, force);
       scheduler.addFiber(fiber.root, err => {
         if (err) {
           reject(err);
@@ -361,6 +358,7 @@ export class Component<T extends Env, Props extends {}> {
         }
         resolve();
       });
+      this.__render(fiber);
     });
   }
 
diff --git a/src/component/fiber.ts b/src/component/fiber.ts
index 1ef8b91c..d368c7dc 100644
--- a/src/component/fiber.ts
+++ b/src/component/fiber.ts
@@ -1,5 +1,6 @@
 import { VNode } from "../vdom/index";
 import { Component } from "./component";
+import { scheduler } from "./scheduler";
 
 /**
  * Owl Fiber Class
@@ -216,7 +217,11 @@ export class Fiber {
         component.catchError!(error);
       });
     } else {
+      // the 3 next lines aim to mark the root fiber as being in error, and
+      // to force it to end, without waiting for its children
+      this.root.counter = 0;
       this.root.error = error;
+      scheduler.flush();
       root.destroy();
     }
   }
diff --git a/src/component/scheduler.ts b/src/component/scheduler.ts
index 87511af6..6b496852 100644
--- a/src/component/scheduler.ts
+++ b/src/component/scheduler.ts
@@ -65,3 +65,6 @@ export class Scheduler {
     });
   }
 }
+
+const raf = window.requestAnimationFrame.bind(window);
+export const scheduler = new Scheduler(raf);
diff --git a/src/context.ts b/src/context.ts
index 9f2f13fa..a7d4db86 100644
--- a/src/context.ts
+++ b/src/context.ts
@@ -1,4 +1,5 @@
-import { Component, scheduler } from "./component/component";
+import { Component } from "./component/component";
+import { scheduler } from "./component/scheduler";
 import { EventBus } from "./core/event_bus";
 import { Observer } from "./core/observer";
 import { onWillUnmount } from "./hooks";
diff --git a/tests/component/component.test.ts b/tests/component/component.test.ts
index 1e09da0a..67c873b2 100644
--- a/tests/component/component.test.ts
+++ b/tests/component/component.test.ts
@@ -4739,6 +4739,26 @@ describe("component error handling (catchError)", () => {
     expect(console.error).toBeCalledTimes(0);
     console.error = consoleError;
   });
+
+  test("a rendering error will reject the render promise (with sub components)", async () => {
+    class Child extends Component<any, any> {
+      static template = xml`<span></span>`;
+    }
+    class Parent extends Component<any, any> {
+      static template = xml`<div><Child/><t t-esc="x.y"/></div>`;
+      static components = { Child };
+    }
+
+    let error;
+    try {
+      const parent = new Parent(env);
+      await parent.mount(fixture);
+    } catch (e) {
+      error = e;
+    }
+    expect(error).toBeDefined();
+    expect(error.message).toBe("Cannot read property 'y' of undefined");
+  });
 });
 
 describe("top level sub widgets", () => {
diff --git a/tests/helpers.ts b/tests/helpers.ts
index d9ddf407..cde909ab 100644
--- a/tests/helpers.ts
+++ b/tests/helpers.ts
@@ -1,4 +1,5 @@
-import { Env, scheduler } from "../src/component/component";
+import { Env } from "../src/component/component";
+import { scheduler } from "../src/component/scheduler";
 import { EvalContext, QWeb } from "../src/qweb/qweb";
 import { patch } from "../src/vdom";
 import "../src/qweb/base_directives";