From dbfc7e4acddca3eeaf925f872ec3597e1147b1df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= Date: Thu, 27 Jun 2019 17:10:00 +0200 Subject: [PATCH] [FIX] store/connect: fix bugs with parent/child connected There were big issues when we use parent/child connected widgets. - children was rendered twice (and mapStoreToProps was called twice) - if children was supposed to be destroyed, it was rendered once. We solve them in this commit by waiting for parent widgets to be ready before updating children. closes #216 --- doc/store.md | 28 +++++ src/store.ts | 57 +++++++--- tests/store.test.ts | 253 +++++++++++++++++++++++++++++++++++++++----- 3 files changed, 300 insertions(+), 38 deletions(-) diff --git a/doc/store.md b/doc/store.md index 259dc216..866e3b1e 100644 --- a/doc/store.md +++ b/doc/store.md @@ -10,6 +10,8 @@ - [Actions](#actions) - [Getters](#getters) - [Connecting a Component](#connecting-a-component) + - [Semantics](#semantics) + - [Good Practices](#good-practices) ## Overview @@ -207,3 +209,29 @@ The arguments of `connect` are: The `connect` function returns a sub class of the given `Component` which is connected to the `store`. + +### Semantics + +The `Store` and the `connect` function try to be smart and to optimize as much +as possible the rendering and update process. What is important to know is: + +- components are always updated in the order of their creation (so, parent + before children) +- they are updated only if they are in the DOM +- if a parent is asynchronous, the system will wait for it to complete its + update before updating other components. +- in general, updates are not coordinated. This is not a problem for synchronous + components, but if there are many asynchronous components, this could lead to + a situation where some part of the UI is updated and other parts of the UI is + not updated. + +### Good Practices + +- avoid asynchronous components as much as possible. Asynchronous components + lead to situations where parts of the UI is not updated immediately. +- do not be afraid to connect many components, parent or children if needed. For + example, a `MessageList` component could get a list of ids in its `mapStoreToProps` and a `Message` component could get the data of its own + message +- since the `mapStoreToProps` function is called for each connected component, + for each state update, it is important to make sure that these functions are + as fast as possible. \ No newline at end of file diff --git a/src/store.ts b/src/store.ts index 5d0e39f9..f8fb34d4 100644 --- a/src/store.ts +++ b/src/store.ts @@ -48,6 +48,7 @@ export class Store extends EventBus { observer: Observer; getters: { [name: string]: (payload?) => any }; _gettersCache: { [name: string]: {} }; + _updateId: number = 1; constructor(config: StoreConfig, options: StoreOption = {}) { super(); @@ -57,10 +58,7 @@ export class Store extends EventBus { this.mutations = config.mutations; this.env = config.env; this.observer = new Observer(); - this.observer.notifyCB = () => { - this._gettersCache = {}; - this.trigger("update"); - }; + this.observer.notifyCB = this.__notifyComponents.bind(this); this.observer.allowMutations = false; this.observer.observe(this.state); this.getters = {}; @@ -139,6 +137,34 @@ export class Store extends EventBus { this._commitLevel--; return res; } + + /** + * Instead of using trigger to emit an update event, we actually implement + * our own function to do that. The reason is that we need to be smarter than + * a simple trigger function: we need to wait for parent components to be + * done before doing children components. The reason is that if an update + * as an effect of destroying a children, we do not want to call the + * mapStoreToProps function of the child, nor rendering it. + * + * This method is not optimal if we have a bunch of asynchronous components: + * we wait sequentially for each component to be completed before updating the + * next. However, the only things that matters is that children are updated + * after their parents. So, this could be optimized by being smarter, and + * updating all widgets concurrently, except for parents/children. + */ + async __notifyComponents() { + this._updateId++; + const current = this._updateId; + this._gettersCache = {}; + const subs = this.subscriptions.update || []; + for (let i = 0, iLen = subs.length; i < iLen; i++) { + const sub = subs[i]; + const shouldCallback = sub.owner ? sub.owner.__owl__.isMounted : true; + if (shouldCallback) { + await sub.callback.call(sub.owner, current); + } + } + } } //------------------------------------------------------------------------------ @@ -231,7 +257,7 @@ export function connect( * if we use the mounted hook, this will be done in the reverse order. */ __callMounted() { - (this.__owl__).store.on("update", this, this._checkUpdate); + (this.__owl__).store.on("update", this, this.__checkUpdate); super.__callMounted(); } willUnmount() { @@ -239,7 +265,10 @@ export function connect( super.willUnmount(); } - _checkUpdate() { + async __checkUpdate(updateId) { + if (updateId === (this.__owl__).currentUpdateId) { + return; + } const ownProps = (this.__owl__).ownProps; const storeProps = mapStoreToProps( (this.__owl__).store.state, @@ -265,22 +294,24 @@ export function connect( } if (didChange) { (this.__owl__).currentStoreProps = storeProps; - this.__updateProps(ownProps, false); + await this.__updateProps(ownProps, false); } } __updateProps(nextProps, forceUpdate, patchQueue?: any[]) { - if ((this.__owl__).ownProps !== nextProps) { - (this.__owl__).currentStoreProps = mapStoreToProps( - (this.__owl__).store.state, + const __owl__ = this.__owl__; + __owl__.currentUpdateId = __owl__.store._updateId; + if (__owl__.ownProps !== nextProps) { + __owl__.currentStoreProps = mapStoreToProps( + __owl__.store.state, nextProps, - (this.__owl__).store.getters + __owl__.store.getters ); } - (this.__owl__).ownProps = nextProps; + __owl__.ownProps = nextProps; const mergedProps = Object.assign( {}, nextProps, - (this.__owl__).currentStoreProps + __owl__.currentStoreProps ); return super.__updateProps(mergedProps, forceUpdate, patchQueue); } diff --git a/tests/store.test.ts b/tests/store.test.ts index 1b3f39d4..2efbaa35 100644 --- a/tests/store.test.ts +++ b/tests/store.test.ts @@ -6,6 +6,7 @@ import { nextMicroTick, nextTick } from "./helpers"; +import { Observer } from "../src"; describe("basic use", () => { test("commit a mutation", () => { @@ -207,7 +208,7 @@ describe("basic use", () => { bestBeerName({ state }) { n++; return state.beers[1].name; - }, + } }; const store = new Store({ state, mutations: {}, actions: {}, getters }); expect((store.getters).bestBeerName()).toBe("bertinchamps"); @@ -226,7 +227,7 @@ describe("basic use", () => { name: "bertinchamps", tasterID: 1 } - }, + } }; let n = 0; const getters = { @@ -265,7 +266,7 @@ describe("basic use", () => { } }; const mutations = { - changeTaster({ state }, {beerID, tasterID}) { + changeTaster({ state }, { beerID, tasterID }) { state.beers[beerID].tasterID = tasterID; } }; @@ -274,15 +275,20 @@ describe("basic use", () => { beerTasterName({ state }, beerID) { n++; return state.tasters[state.beers[beerID].tasterID].name; - }, + } }; - const store = new Store({ state, mutations: mutations, actions: {}, getters }); + const store = new Store({ + state, + mutations: mutations, + actions: {}, + getters + }); expect((store.getters).beerTasterName(1)).toBe("aaron"); expect(n).toBe(1); expect((store.getters).beerTasterName(1)).toBe("aaron"); expect(n).toBe(1); - store.commit('changeTaster', {beerID: 1, tasterID: 2}); + store.commit("changeTaster", { beerID: 1, tasterID: 2 }); await nextTick(); expect((store.getters).beerTasterName(1)).toBe("gery"); @@ -295,14 +301,14 @@ describe("basic use", () => { 1: { id: 1, name: "bertinchamps" - }, + } } }; const mutations = { renameBeer({ state, getters }, beerID) { - expect(getters.beerName(beerID)).toBe('bertinchamps'); - state.beers[1].name = 'chouffe'; - expect(getters.beerName(beerID)).toBe('chouffe'); + expect(getters.beerName(beerID)).toBe("bertinchamps"); + state.beers[1].name = "chouffe"; + expect(getters.beerName(beerID)).toBe("chouffe"); } }; let n = 0; @@ -310,12 +316,17 @@ describe("basic use", () => { beerName({ state }, beerID) { n++; return state.beers[beerID].name; - }, + } }; - const store = new Store({ state, mutations: mutations, actions: {}, getters }); + const store = new Store({ + state, + mutations: mutations, + actions: {}, + getters + }); - store.commit('renameBeer', 1); - expect((store.getters).beerName(1)).toBe('chouffe'); + store.commit("renameBeer", 1); + expect((store.getters).beerName(1)).toBe("chouffe"); await nextTick(); expect(n).toBe(3); @@ -1117,7 +1128,190 @@ describe("connecting a component to store", () => { store.commit("setCurrent", "b"); await nextTick(); - expect(steps).toEqual(["parent", "child", "parent", "child", "child"]); + expect(fixture.innerHTML).toBe("
b
"); + expect(steps).toEqual(["parent", "child", "parent", "child"]); + }); + + test("connected parent/children: no double rendering", async () => { + const mutations = { + editTodo({ state }) { + state.todos[1].title = "abc"; + } + }; + const todos = { 1: { id: 1, title: "kikoou" } }; + const state = { + todos + }; + const store = new Store({ + state, + mutations + }); + + env.qweb.addTemplates(` + +
+ + + +
+ +
+ + +
+ + `); + + function mapStoreToPropsTodoApp(state) { + return { + todos: state.todos + }; + } + + class TodoApp extends Component { + components = { ConnectedTodoItem }; + } + + const ConnectedTodoApp = connect( + TodoApp, + mapStoreToPropsTodoApp + ); + + let renderCount = 0; + let fCount = 0; + + function mapStoreToPropsTodoItem(state, ownProps) { + fCount++; + return { + todo: state.todos[ownProps.id] + }; + } + + class TodoItem extends Component { + state = { isEditing: false }; + + editTodo() { + this.env.store.commit("editTodo"); + } + __render(...args) { + renderCount++; + return super.__render(...args); + } + } + + const ConnectedTodoItem = connect( + TodoItem, + mapStoreToPropsTodoItem + ); + + (env).store = store; + const app = new ConnectedTodoApp(env); + + await app.mount(fixture); + expect(fixture.innerHTML).toBe( + '
kikoou
' + ); + + expect(renderCount).toBe(1); + expect(fCount).toBe(1); + fixture.querySelector("button")!.click(); + await nextTick(); + expect(renderCount).toBe(2); + expect(fCount).toBe(2); + expect(fixture.innerHTML).toBe( + '
abc
' + ); + }); + + test("connected parent/children: no rendering if child is destroyed", async () => { + const mutations = { + removeTodo({ state }) { + Observer.delete(state.todos, 1); + } + }; + const todos = { 1: { id: 1, title: "kikoou" } }; + const state = { + todos + }; + const store = new Store({ + state, + mutations + }); + + env.qweb.addTemplates(` + +
+ + + +
+ +
+ + +
+ + `); + + function mapStoreToPropsTodoApp(state) { + return { + todos: state.todos + }; + } + + class TodoApp extends Component { + components = { ConnectedTodoItem }; + } + + const ConnectedTodoApp = connect( + TodoApp, + mapStoreToPropsTodoApp + ); + + let renderCount = 0; + let fCount = 0; + + function mapStoreToPropsTodoItem(state, ownProps) { + fCount++; + return { + todo: state.todos[ownProps.id] + }; + } + + class TodoItem extends Component { + state = { isEditing: false }; + + removeTodo() { + this.env.store.commit("removeTodo"); + } + __render(...args) { + renderCount++; + return super.__render(...args); + } + } + + const ConnectedTodoItem = connect( + TodoItem, + mapStoreToPropsTodoItem + ); + + (env).store = store; + const app = new ConnectedTodoApp(env); + + await app.mount(fixture); + expect(fixture.innerHTML).toBe( + '
kikoou
' + ); + + expect(renderCount).toBe(1); + expect(fCount).toBe(1); + fixture.querySelector("button")!.click(); + await nextTick(); + expect(renderCount).toBe(1); + expect(fCount).toBe(1); + expect(fixture.innerHTML).toBe( + '
' + ); }); test("connected component willpatch/patch hooks are called on store updates", async () => { @@ -1159,19 +1353,28 @@ describe("connecting a component to store", () => { }); test("connected component has its own name", () => { - function mapStoreToProps() { } + function mapStoreToProps() {} - class Named extends Component { }; - const namedConnected = connect(Named, mapStoreToProps); - expect(namedConnected.name).toMatch('ConnectedNamed'); + class Named extends Component {} + const namedConnected = connect( + Named, + mapStoreToProps + ); + expect(namedConnected.name).toMatch("ConnectedNamed"); - class ParentNamed extends Component{}; - class ChildNamed extends ParentNamed{}; - const childConnected = connect(ChildNamed, mapStoreToProps) - expect(childConnected.name).toMatch('ConnectedChildNamed') + class ParentNamed extends Component {} + class ChildNamed extends ParentNamed {} + const childConnected = connect( + ChildNamed, + mapStoreToProps + ); + expect(childConnected.name).toMatch("ConnectedChildNamed"); - const Anonymous = class extends Component{ }; - const anonymousConnected = connect(Anonymous, mapStoreToProps); + const Anonymous = class extends Component {}; + const anonymousConnected = connect( + Anonymous, + mapStoreToProps + ); expect(anonymousConnected.name).toMatch(/^Connectedclass_\d+/); }); });