From d043d47754f4755d5ccb8505ee4f51c4be4ce66d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= Date: Wed, 6 Jan 2021 15:20:55 +0100 Subject: [PATCH] [FIX] component: propagate correct info when reusing fibers In some cases, a rendering initiated in some component is then remapped into a larger rendering initiated by some parent. If we have some components which implement shouldUpdate to return false, then the following scenario can happen: - some parent component is mounted (which triggers a rendering with force: true => bypass the shouldUpdate) - some sub component is updated and rerendered, AFTER the previous rendering goes through it - the sub component notices that there is an ongoing rendering, and remaps itself in the parent rendering Before this commit, the new fiber in the subcomponent does not have force flag set to true, so the new rendering for the subcomponent does not go through its own children (if they have shouldUpdate=false) Another more complex kind of scenaria can happen when a remapped rendering happen with sub components with dynamic shouldUpdate. The problem is the same at the end: the new rendering should ignore the shouldUpdate, to make sure we have the last correct information. With this commit, we make sure that the flag of the new fiber is set to true. closes #818 --- doc/reference/component.md | 19 ++++- src/component/fiber.ts | 1 + tests/component/async.test.ts | 156 ++++++++++++++++++++++++++++++++++ 3 files changed, 173 insertions(+), 3 deletions(-) diff --git a/doc/reference/component.md b/doc/reference/component.md index 9510c6b4..6ebb72ef 100644 --- a/doc/reference/component.md +++ b/doc/reference/component.md @@ -10,13 +10,21 @@ - [Static Properties](#static-properties) - [Methods](#methods) - [Lifecycle](#lifecycle) + - [`constructor(parent, props)`](#constructorparent-props) + - [`willStart()`](#willstart) + - [`mounted()`](#mounted) + - [`willUpdateProps(nextProps)`](#willupdatepropsnextprops) + - [`willPatch()`](#willpatch) + - [`patched(snapshot)`](#patchedsnapshot) + - [`willUnmount()`](#willunmount) + - [`catchError(error)`](#catcherrorerror) - [Root Component](#root-component) - [Composition](#composition) - [Form Input Bindings](#form-input-bindings) - [References](#references) - [Dynamic sub components](#dynamic-sub-components) - [Functional Components](#functional-components) - - [SVG components](#svg-components) + - [SVG Components](#svg-components) ## Overview @@ -289,8 +297,13 @@ We explain here all the public methods of the `Component` class. are updated. It returns a boolean, which indicates if the component should ignore a props update. If it returns false, then `willUpdateProps` will not be called, and no rendering will occur. Its default implementation is to - always return true. This is an optimization, similar to React's `shouldComponentUpdate`. Most of the time, this should not be used, but it - can be useful if we are handling large number of components. + always return true. Note that this is an optimization, similar to React's `shouldComponentUpdate`. Most of the time, this should not be used, but it + can be useful if we are handling large number of components. Since this is an + optimization, Owl has the freedom to ignore the result of `shouldUpdate` in + some cases (for example, if a component is remounted, or if we want to force + a full rerender of the UI). However, if `shouldUpdate` returns true, then Owl + provides the guarantee that the component will be rendered at some point in + the future (except if the component is destroyed or if some part of the UI crashes). * **`destroy()`**. As its name suggests, this method will remove the component, and perform all necessary cleanup, such as unmounting the component, its children, diff --git a/src/component/fiber.ts b/src/component/fiber.ts index e981b9d8..b19d10eb 100644 --- a/src/component/fiber.ts +++ b/src/component/fiber.ts @@ -82,6 +82,7 @@ export class Fiber { let oldFiber = __owl__.currentFiber; if (oldFiber && !oldFiber.isCompleted) { + this.force = true; if (oldFiber.root === oldFiber && !parent) { // both oldFiber and this fiber are root fibers this._reuseFiber(oldFiber); diff --git a/tests/component/async.test.ts b/tests/component/async.test.ts index 7644c61a..469f0eeb 100644 --- a/tests/component/async.test.ts +++ b/tests/component/async.test.ts @@ -1405,4 +1405,160 @@ describe("async rendering", () => { expect(fixture.innerHTML).toBe("
2
"); expect(Widget.prototype.__render).toHaveBeenCalledTimes(2); }); + + test("components with shouldUpdate=false", async () => { + const state = { p: 1, cc: 10 }; + + class ChildChild extends Component { + static template = xml` +
+ child child: +
`; + state = state; + shouldUpdate() { + return false; + } + } + + class Child extends Component { + static components = { ChildChild }; + static template = xml` +
+ child + +
`; + + shouldUpdate() { + return false; + } + } + + let parent: any; + class Parent extends Component { + static components = { Child }; + static template = xml` +
+ parent: + +
`; + + state = state; + constructor(a, b) { + super(a, b); + parent = this; + } + shouldUpdate() { + return false; + } + } + + class App extends Component { + static components = { Parent }; + static template = xml` +
+ +
`; + } + + var div = document.createElement("div"); + fixture.appendChild(div); + + const app = new App(); + + await app.mount(fixture); + expect(fixture.innerHTML).toBe( + "
parent: 1
child
child child: 10
" + ); + app.mount(div); + + // wait for rendering from second mount to go through parent + await Promise.resolve(); + await Promise.resolve(); + state.cc++; + state.p++; + parent.render(); + await nextTick(); + expect(fixture.innerHTML).toBe( + "
parent: 2
child
child child: 11
" + ); + }); + + test("components with shouldUpdate=false, part 2", async () => { + const state = { p: 1, cc: 10 }; + let shouldUpdate = true; + + class ChildChild extends Component { + static template = xml` +
+ child child: +
`; + state = state; + shouldUpdate() { + return shouldUpdate; + } + } + + class Child extends Component { + static components = { ChildChild }; + static template = xml` +
+ child + +
`; + + shouldUpdate() { + return shouldUpdate; + } + } + + let parent: any; + class Parent extends Component { + static components = { Child }; + static template = xml` +
+ parent: + +
`; + + state = state; + constructor(a, b) { + super(a, b); + parent = this; + } + shouldUpdate() { + return shouldUpdate; + } + } + + class App extends Component { + static components = { Parent }; + static template = xml` +
+ +
`; + } + + const app = new App(); + + await app.mount(fixture); + expect(fixture.innerHTML).toBe( + "
parent: 1
child
child child: 10
" + ); + + state.cc++; + state.p++; + app.render(); + + // wait for rendering to go through child + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + + shouldUpdate = false; + parent.render(); + await nextTick(); + expect(fixture.innerHTML).toBe( + "
parent: 2
child
child child: 11
" + ); + }); });