diff --git a/src/context.ts b/src/context.ts index 2ae74e38..10e64477 100644 --- a/src/context.ts +++ b/src/context.ts @@ -115,16 +115,6 @@ export function useContextWithCB(ctx: Context, component: Component, method): an __owl__.observer = new Observer(); __owl__.observer.notifyCB = component.render.bind(component); } - const currentCB = __owl__.observer.notifyCB; - __owl__.observer.notifyCB = function () { - if (ctx.rev > mapping[id]) { - // in this case, the context has been updated since we were rendering - // last, and we do not need to render here with the observer. A - // rendering is coming anyway, with the correct props. - return; - } - currentCB(); - }; mapping[id] = 0; const renderFn = __owl__.renderFn; diff --git a/src/store.ts b/src/store.ts index 8bfa5632..ead4a763 100644 --- a/src/store.ts +++ b/src/store.ts @@ -1,5 +1,4 @@ -import { Component } from "./component/component"; -import { Env } from "./component/component"; +import { Component, Env } from "./component/component"; import { Context, useContextWithCB } from "./context"; import { onWillUpdateProps } from "./hooks"; diff --git a/tests/context.test.ts b/tests/context.test.ts index b168e48c..9e9b7226 100644 --- a/tests/context.test.ts +++ b/tests/context.test.ts @@ -289,7 +289,26 @@ describe("Context", () => { expect(testContext.subscriptions.update.length).toBe(0); }); - test("concurrent renderings", async () => { + test.skip("concurrent renderings", async () => { + /** + * Note: this test is interesting, but sadly just an incomplete attempt at + * protecting users against themselves. With the context API, it is not + * possible for the framework to protect completely against crashes. Maybe + * like in this case, when a component is in a simple hierarchy where all + * renderings come from the context changes, but in a real case, where some + * code can trigger a rendering independently, it is insufficient. + * + * The main problem is that the sub component depends on some external state, + * which may be modified, and then incompatible with the component actual + * state (for example, if the sub component has an id key related to some + * object that has been removed from the context). + * + * For now, sadly, the only solution is that components that depends on external + * state should guarantee their own integrity themselves. Then maybe this + * could be solved at the level of a state management solution that has a + * more advanced API, to let components determine if they should be updated + * or not (so, something slightly more advanced that the useStore hook). + */ const testContext = new Context({ x: { n: 1 }, key: "x" }); const def = makeDeferred(); let stateC; diff --git a/tests/store_hooks.test.ts b/tests/store_hooks.test.ts index 76616425..3a54d540 100644 --- a/tests/store_hooks.test.ts +++ b/tests/store_hooks.test.ts @@ -1,4 +1,4 @@ -import { Component, Env } from "../src/component/component"; +import { Component, Env, mount } from "../src/component/component"; import { Store, useStore, useDispatch, useGetters, EnvWithStore } from "../src/store"; import { useState } from "../src/hooks"; import { xml } from "../src/tags"; @@ -1241,4 +1241,108 @@ describe("various scenarios", () => { await nextTick(); expect(fixture.innerHTML).toMatchSnapshot(); }); + + test("component with store, useState and shouldUpdate=false", async () => { + let state: any; + + const store = new Store({ state: { rev: 0 } }); + + class Child extends Component { + static template = xml`
`; + + state = useState({ word: "hello" }); + + constructor(parent, props) { + super(parent, props); + state = this.state; + useStore((props) => { + return 1; + }); + } + + shouldUpdate() { + return false; + } + } + + class Parent extends Component { + static template = xml`
`; + static components = { Child }; + + state = useState({ name: "World" }); + + constructor(parent, props) { + super(parent, props); + useStore((props) => store.state.rev); + } + } + (env as any).store = store; + + await mount(Parent, { target: fixture, env }); + expect(fixture.innerHTML).toBe("
helloWorld
"); + + store.state.rev++; + // this is the key to the bug, it makes Parent be in "render" state but not + // yet rendered while the change of state happens + await Promise.resolve(); + state.word = "test"; + await nextTick(); + expect(fixture.innerHTML).toBe("
testWorld
"); + }); + + test("component with store, useState, shouldUpdate=false and child with shouldupdate false", async () => { + let state: any; + + const store = new Store({ state: { rev: 0 } }); + + class ChildChild extends Component { + static template = xml`
`; + + shouldUpdate() { + return false; + } + } + + class Child extends Component { + static template = xml`
`; + static components = { ChildChild }; + state = useState({ word: "hello", value: 3 }); + + constructor(parent, props) { + super(parent, props); + state = this.state; + useStore((props) => { + return 1; + }); + } + + shouldUpdate() { + return false; + } + } + + class Parent extends Component { + static template = xml`
`; + static components = { Child }; + + state = useState({ name: "World" }); + constructor(parent, props) { + super(parent, props); + useStore((props) => store.state.rev); + } + } + (env as any).store = store; + + await mount(Parent, { target: fixture, env }); + expect(fixture.innerHTML).toBe("
helloWorld
3
"); + + store.state.rev++; + // this is the key to the bug, it makes Parent be in "render" state but not + // yet rendered while the change of state happens + await Promise.resolve(); + state.word = "test"; + state.value = 44; + await nextTick(); + expect(fixture.innerHTML).toBe("
testWorld
3
"); + }); });