From d9cbd23cd9444521e95ec1555f86c7a673a8459b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= Date: Tue, 9 Jul 2019 14:33:56 +0200 Subject: [PATCH] [IMP] component: add catchError hook --- doc/component.md | 85 +++++++++-- src/component.ts | 108 ++++++++++---- tests/component.test.ts | 324 ++++++++++++++++++++++++++++++++++++++-- 3 files changed, 468 insertions(+), 49 deletions(-) diff --git a/doc/component.md b/doc/component.md index 509105c0f..a12cf4dec 100644 --- a/doc/component.md +++ b/doc/component.md @@ -21,6 +21,7 @@ - [References](#references) - [Slots](#slots) - [Asynchronous Rendering](#asynchronous-rendering) + - [Error Handling](#error-handling) ## Overview @@ -188,15 +189,16 @@ A solid and robust component system needs useful hooks/methods to help developers write components. Here is a complete description of the lifecycle of a owl component: -| Method | Description | -| ------------------------------------------------ | ----------------------------------------------------- | -| **[constructor](#constructorparent-props)** | constructor | -| **[willStart](#willstart)** | async, before first rendering | -| **[mounted](#mounted)** | just after component is rendered and added to the DOM | -| **[willUpdateProps](#willupdatepropsnextprops)** | async, before props update | -| **[willPatch](#willpatch)** | just before the DOM is patched | -| **[patched](#patchedsnapshot)** | just after the DOM is patched | -| **[willUnmount](#willunmount)** | just before removing component from DOM | +| Method | Description | +| ------------------------------------------------ | ------------------------------------------------------------ | +| **[constructor](#constructorparent-props)** | constructor | +| **[willStart](#willstart)** | async, before first rendering | +| **[mounted](#mounted)** | just after component is rendered and added to the DOM | +| **[willUpdateProps](#willupdatepropsnextprops)** | async, before props update | +| **[willPatch](#willpatch)** | just before the DOM is patched | +| **[patched](#patchedsnapshot)** | just after the DOM is patched | +| **[willUnmount](#willunmount)** | just before removing component from DOM | +| **[catchError](#catcherrorerror)** | catch errors (see [error handling section](#error-handling)) | Notes: @@ -337,6 +339,12 @@ the DOM. This is a good place to remove some listeners, for example. This is the opposite method of `mounted`. +#### `catchError(error)` + +The `catchError` method is useful when we need to intercept and properly react +to (rendering) errors that occur in some sub components. See the section on +[error handling](#error-handling) + ### Root Component Most of the time, an Owl component will be created automatically by a tag (or the `t-component` @@ -1020,3 +1028,62 @@ Here are a few tips on how to work with asynchronous components: ``` + +### Error Handling + +By default, whenever an error occurs in the rendering of an Owl application, we +destroy the whole application. Otherwise, we cannot offer any guarantee on the +state of the resulting component tree. It might be hopelessly corrupted, but +without any user-visible state. + +Clearly, it sometimes is a little bit extreme to destroy the application. This +is why we have a builtin mechanism to handle rendering errors (and errors coming +from lifecycle hooks): the `catchError` hook. + +Whenever the `catchError` lifecycle hook is implemented, all errors coming from +sub components rendering and/or lifecycle method calls will be caught and given +to the `catchError` method. This allow us to properly handle the error, and to +not break the application. + +For example, here is how we could implement an `ErrorBoundary` component: + +```xml +
+ + Error handled + + + + +
+``` + +```js +class ErrorBoundary extends Widget { + state = { error: false }; + + catchError() { + this.state.error = true; + } +} +``` + +Using the `ErrorBoundary` is then extremely simple: + +```xml + +``` + +Note that we need to be careful here: the fallback UI should not throw any +error, otherwise we risk going into an infinite loop. + +Also, it may be useful to know that whenever an error is caught, it is then +broadcasted to the application by an event on the `qweb` instance. It may be +useful, for example, to log the error somewhere. + +```js +env.qweb.on("error", null, function(error) { + // do something + // react to the error +}); +``` diff --git a/src/component.ts b/src/component.ts index b1fa315f4..f12cd5e46 100644 --- a/src/component.ts +++ b/src/component.ts @@ -243,6 +243,12 @@ export class Component { */ willUnmount() {} + /** + * catchError is a method called whenever some error happens in the rendering or + * lifecycle hooks of a child. + */ + catchError(error: Error): void {} + //-------------------------------------------------------------------------- // Public //-------------------------------------------------------------------------- @@ -398,7 +404,11 @@ export class Component { for (let key in handlers) { handlers[key](); } - this.mounted(); + try { + this.mounted(); + } catch (e) { + errorHandler(e, this); + } } __callWillUnmount() { @@ -419,7 +429,7 @@ export class Component { forceUpdate: boolean = false, patchQueue?: any[], scope?: any, - vars?: any, + vars?: any ): Promise { const shouldUpdate = forceUpdate || this.shouldUpdate(nextProps); if (shouldUpdate) { @@ -451,7 +461,12 @@ export class Component { } async __prepareAndRender(scope?: Object, vars?: any): Promise { - await this.willStart(); + try { + await this.willStart(); + } catch (e) { + errorHandler(e, this); + return Promise.resolve(h("div")); + } const __owl__ = this.__owl__; if (__owl__.isDestroyed) { return Promise.resolve(h("div")); @@ -485,7 +500,12 @@ export class Component { return this.__render(false, [], scope, vars); } - async __render(force: boolean = false, patchQueue: any[] = [], scope?: Object, vars?: any): Promise { + async __render( + force: boolean = false, + patchQueue: any[] = [], + scope?: Object, + vars?: any + ): Promise { const __owl__ = this.__owl__; __owl__.renderId++; const promises: Promise[] = []; @@ -496,15 +516,21 @@ export class Component { if (__owl__.observer) { __owl__.observer.allowMutations = false; } - let vnode = __owl__.render!(this, { - promises, - handlers: __owl__.boundHandlers, - mountedHandlers: __owl__.mountedHandlers, - forceUpdate: force, - patchQueue, - scope, - vars - }); + let vnode; + try { + vnode = __owl__.render!(this, { + promises, + handlers: __owl__.boundHandlers, + mountedHandlers: __owl__.mountedHandlers, + forceUpdate: force, + patchQueue, + scope, + vars + }); + } catch (e) { + vnode = __owl__.vnode || h("div"); + errorHandler(e, this); + } patch.push(vnode); if (__owl__.observer) { __owl__.observer.allowMutations = true; @@ -580,18 +606,50 @@ export class Component { * 3) Call 'patched' on the component of each patch, in inverse order */ __applyPatchQueue(patchQueue: any[]) { - const patchLen = patchQueue.length; - for (let i = 0; i < patchLen; i++) { - const patch = patchQueue[i]; - patch.push(patch[0].willPatch()); - } - for (let i = 0; i < patchLen; i++) { - const patch = patchQueue[i]; - patch[0].__patch(patch[1]); - } - for (let i = patchLen - 1; i >= 0; i--) { - const patch = patchQueue[i]; - patch[0].patched(patch[2]); + let component = this; + try { + const patchLen = patchQueue.length; + for (let i = 0; i < patchLen; i++) { + const patch = patchQueue[i]; + component = patch[0]; + patch.push(patch[0].willPatch()); + } + for (let i = 0; i < patchLen; i++) { + const patch = patchQueue[i]; + patch[0].__patch(patch[1]); + } + for (let i = patchLen - 1; i >= 0; i--) { + const patch = patchQueue[i]; + component = patch[0]; + patch[0].patched(patch[2]); + } + } catch (e) { + errorHandler(e, component); } } } + +//------------------------------------------------------------------------------ +// Error handling +//------------------------------------------------------------------------------ + +function errorHandler(error, component) { + let canCatch = false; + let qweb = component.env.qweb; + let root = component; + while (component && !(canCatch = component.catchError !== Component.prototype.catchError)) { + root = component; + component = component.__owl__.parent; + } + console.error(error); + // we trigger error on QWeb so it can be logged/handled + qweb.trigger("error", error); + + if (canCatch) { + setTimeout(() => { + component.catchError(error); + }); + } else { + root.destroy(); + } +} diff --git a/tests/component.test.ts b/tests/component.test.ts index 3dd488eee..c3feee48e 100644 --- a/tests/component.test.ts +++ b/tests/component.test.ts @@ -879,18 +879,22 @@ describe("composition", () => { expect(fixture.innerHTML).toBe("
1
"); }); - test("throw a nice error if it cannot find component", async () => { - expect.assertions(1); + test("display a nice error if it cannot find component", async () => { + const consoleError = console.error; + console.error = jest.fn(); + env.qweb.addTemplate("Parent", `
`); class Parent extends Widget { components = { SomeWidget: Widget }; } const parent = new Parent(env); - try { - await parent.mount(fixture); - } catch (e) { - expect(e.message).toBe('Cannot find the definition of component "SomeMispelledWidget"'); - } + await parent.mount(fixture); + + expect(console.error).toBeCalledTimes(1); + expect((console.error).mock.calls[0][0].message).toMatch( + 'Cannot find the definition of component "SomeMispelledWidget"' + ); + console.error = consoleError; }); test("t-refs on widget are components", async () => { @@ -2726,7 +2730,8 @@ describe("widget and observable state", () => { }); test("subcomponents cannot change observable state received from parent", async () => { - expect.assertions(1); + const consoleError = console.error; + console.error = jest.fn(); env.qweb.addTemplate("Parent", `
`); class Parent extends Widget { state = { obj: { coffee: 1 } }; @@ -2739,11 +2744,13 @@ describe("widget and observable state", () => { } } const parent = new Parent(env); - try { - await parent.mount(fixture); - } catch (e) { - expect(e.message).toBe('Observed state cannot be changed here! (key: "coffee", val: "2")'); - } + await parent.mount(fixture); + + expect(console.error).toBeCalledTimes(1); + expect((console.error).mock.calls[0][0].message).toMatch( + 'Observed state cannot be changed here! (key: "coffee", val: "2")' + ); + console.error = consoleError; }); }); @@ -3260,10 +3267,9 @@ describe("t-slot directive", () => { await app.mount(fixture); expect(fixture.innerHTML).toBe("
SC:4
"); - (fixture.querySelector('button')).click(); + (fixture.querySelector("button")).click(); await nextTick(); expect(fixture.innerHTML).toBe("
SC:5
"); - }); }); @@ -3525,3 +3531,291 @@ describe("environment and plugins", () => { expect(fixture.innerHTML).toBe("
Blue
"); }); }); + +describe("component error handling (catchError)", () => { + /** + * This test suite requires often to wait for 3 ticks. Here is why: + * - First tick is to let the app render and crash. + * - When we crash, we call the catchError handler in a setTimeout (because we + * need to wait for the previous rendering to be completely stopped). So, we + * need to wait for the second tick. + * - Then, when the handler changes the state, we need to wait for the interface + * to be rerendered. + * */ + + test("can catch an error in a component render function", async () => { + const consoleError = console.error; + console.error = jest.fn(); + env.qweb.addTemplates(` + +
+ Error handled + +
+
hey +
+
+ +
+ `); + const handler = jest.fn(); + env.qweb.on("error", null, handler); + class ErrorComponent extends Widget {} + class ErrorBoundary extends Widget { + state = { error: false }; + + catchError() { + this.state.error = true; + } + } + class App extends Widget { + state = { flag: false }; + components = { ErrorBoundary, ErrorComponent }; + } + const app = new App(env); + await app.mount(fixture); + expect(fixture.innerHTML).toBe("
hey
"); + app.state.flag = true; + await nextTick(); + await nextTick(); + await nextTick(); + expect(fixture.innerHTML).toBe("
Error handled
"); + + expect(console.error).toBeCalledTimes(1); + console.error = consoleError; + expect(handler).toBeCalledTimes(1); + }); + + test("no component catching error lead to full app destruction", async () => { + const handler = jest.fn(); + env.qweb.on("error", null, handler); + const consoleError = console.error; + console.error = jest.fn(); + env.qweb.addTemplates(` + +
hey +
+
+ +
+ `); + class ErrorComponent extends Widget {} + class App extends Widget { + state = { flag: false }; + components = { ErrorComponent }; + } + const app = new App(env); + await app.mount(fixture); + expect(fixture.innerHTML).toBe("
hey
"); + app.state.flag = true; + await nextTick(); + await nextTick(); + await nextTick(); + expect(fixture.innerHTML).toBe(""); + + expect(console.error).toBeCalledTimes(1); + console.error = consoleError; + expect(app.__owl__.isDestroyed).toBe(true); + expect(handler).toBeCalledTimes(1); + }); + + test("can catch an error in the initial call of a component render function", async () => { + const handler = jest.fn(); + env.qweb.on("error", null, handler); + const consoleError = console.error; + console.error = jest.fn(); + env.qweb.addTemplates(` + +
+ Error handled + +
+
hey +
+
+ +
+ `); + class ErrorComponent extends Widget {} + class ErrorBoundary extends Widget { + state = { error: false }; + + catchError() { + this.state.error = true; + } + } + class App extends Widget { + components = { ErrorBoundary, ErrorComponent }; + } + const app = new App(env); + await app.mount(fixture); + await nextTick(); + await nextTick(); + await nextTick(); + expect(fixture.innerHTML).toBe("
Error handled
"); + + expect(console.error).toBeCalledTimes(1); + console.error = consoleError; + expect(handler).toBeCalledTimes(1); + }); + + test("can catch an error in the constructor call of a component render function", async () => { + const handler = jest.fn(); + env.qweb.on("error", null, handler); + const consoleError = console.error; + console.error = jest.fn(); + env.qweb.addTemplates(` + +
+ Error handled + +
+
Some text
+
+ +
+ `); + class ErrorComponent extends Widget { + constructor(parent) { + super(parent); + throw new Error("NOOOOO"); + } + } + class ErrorBoundary extends Widget { + state = { error: false }; + + catchError() { + this.state.error = true; + } + } + class App extends Widget { + components = { ErrorBoundary, ErrorComponent }; + } + const app = new App(env); + await app.mount(fixture); + await nextTick(); + await nextTick(); + await nextTick(); + expect(fixture.innerHTML).toBe("
Error handled
"); + + expect(console.error).toBeCalledTimes(1); + console.error = consoleError; + expect(handler).toBeCalledTimes(1); + }); + + test("can catch an error in the willStart call", async () => { + const consoleError = console.error; + console.error = jest.fn(); + env.qweb.addTemplates(` + +
+ Error handled + +
+
Some text
+
+ +
+ `); + class ErrorComponent extends Widget { + async willStart() { + // we wait a little bit to be in a different stack frame + await nextTick(); + throw new Error("NOOOOO"); + } + } + class ErrorBoundary extends Widget { + state = { error: false }; + + catchError() { + this.state.error = true; + } + } + class App extends Widget { + components = { ErrorBoundary, ErrorComponent }; + } + const app = new App(env); + await app.mount(fixture); + await nextTick(); + await nextTick(); + await nextTick(); + expect(fixture.innerHTML).toBe("
Error handled
"); + + expect(console.error).toBeCalledTimes(1); + console.error = consoleError; + }); + + test("can catch an error in the mounted call", async () => { + console.error = jest.fn(); + env.qweb.addTemplates(` + +
+ Error handled + +
+
Some text
+
+ +
+ `); + class ErrorComponent extends Widget { + mounted() { + throw new Error("NOOOOO"); + } + } + class ErrorBoundary extends Widget { + state = { error: false }; + + catchError() { + this.state.error = true; + } + } + class App extends Widget { + components = { ErrorBoundary, ErrorComponent }; + } + const app = new App(env); + await app.mount(fixture); + await nextTick(); + await nextTick(); + await nextTick(); + expect(fixture.innerHTML).toBe("
Error handled
"); + }); + + test("can catch an error in the willPatch call", async () => { + env.qweb.addTemplates(` + +
+ Error handled + +
+
+
+ +
+ `); + class ErrorComponent extends Widget { + willPatch() { + throw new Error("NOOOOO"); + } + } + class ErrorBoundary extends Widget { + state = { error: false }; + + catchError() { + this.state.error = true; + } + } + class App extends Widget { + state = { message: "abc" }; + components = { ErrorBoundary, ErrorComponent }; + } + const app = new App(env); + await app.mount(fixture); + expect(fixture.innerHTML).toBe("
abc
"); + app.state.message = "def"; + await nextTick(); + await nextTick(); + await nextTick(); + expect(fixture.innerHTML).toBe("
Error handled
"); + }); +});