From 56087b95cde2a785d12b74fd12e738b0e329dbc7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= Date: Thu, 14 Nov 2019 14:09:42 +0100 Subject: [PATCH] [REF] observer: notify subscribers directly This rev. moves the logic of batching the notifications of changes occurring in the same microtask tick, from the observer, to the listeners (the context, and the render function of components). By doing so in render, we ensure that similar renderings are batched in a single one, wherever they come from (state change, store change, direct call...). --- src/component/component.ts | 17 ++++++++++++++++- src/component/fiber.ts | 6 ++---- src/component/scheduler.ts | 3 +-- src/context.ts | 11 ++++++++++- src/core/observer.ts | 12 ++---------- src/hooks.ts | 12 +----------- tests/component/component.test.ts | 4 ---- tests/core/observer.test.ts | 2 +- 8 files changed, 33 insertions(+), 34 deletions(-) diff --git a/src/component/component.ts b/src/component/component.ts index df014627..c92860de 100644 --- a/src/component/component.ts +++ b/src/component/component.ts @@ -328,8 +328,23 @@ export class Component { if (__owl__.currentFiber && !__owl__.currentFiber.isRendered) { return scheduler.addFiber(__owl__.currentFiber.root); } + // if we aren't mounted at this point, it implies that there is a + // currentFiber that is already rendered (isRendered is true), so we are + // about to be mounted + const isMounted = __owl__.isMounted; const fiber = new Fiber(null, this, undefined, undefined, force, null); - this.__render(fiber); + Promise.resolve().then(() => { + if (__owl__.isMounted || !isMounted) { + // we are mounted (__owl__.isMounted), or if we are currently being + // mounted (!isMounted), so we call __render + this.__render(fiber); + } else { + // we were mounted when render was called, but we aren't anymore, so we + // were actually about to be unmounted ; we can thus forget about this + // fiber + __owl__.currentFiber = null; + } + }); return scheduler.addFiber(fiber); } diff --git a/src/component/fiber.ts b/src/component/fiber.ts index 5a741eaf..f1541f96 100644 --- a/src/component/fiber.ts +++ b/src/component/fiber.ts @@ -173,10 +173,8 @@ export class Fiber { if (document.body.contains(this.target)) { component.__callMounted(); } - } else { - if (component.__owl__.isMounted && this === this.root) { - this.patchComponents(); - } + } else if (component.__owl__.isMounted && this === this.root) { + this.patchComponents(); } this.isCompleted = true; } diff --git a/src/component/scheduler.ts b/src/component/scheduler.ts index 917f1bc5..f486bc33 100644 --- a/src/component/scheduler.ts +++ b/src/component/scheduler.ts @@ -34,8 +34,7 @@ export class Scheduler { fiber, callback: () => { if (fiber.error) { - reject(fiber.error); - return; + return reject(fiber.error); } resolve(); } diff --git a/src/context.ts b/src/context.ts index a7d4db86..84848515 100644 --- a/src/context.ts +++ b/src/context.ts @@ -45,7 +45,16 @@ export class Context extends EventBus { constructor(state: Object = {}) { super(); this.observer = new Observer(); - this.observer.notifyCB = this.__notifyComponents.bind(this); + this.observer.notifyCB = () => { + // notify components in the next microtask tick to ensure that subscribers + // are notified only once for all changes that occur in the same micro tick + let rev = this.rev; + return Promise.resolve().then(() => { + if (rev === this.rev) { + this.__notifyComponents(); + } + }); + }; this.state = this.observer.observe(state); this.subscriptions.update = []; } diff --git a/src/core/observer.ts b/src/core/observer.ts index 39afdbb6..4f9f2a45 100644 --- a/src/core/observer.ts +++ b/src/core/observer.ts @@ -24,14 +24,6 @@ export class Observer { weakMap: WeakMap = new WeakMap(); notifyCB() {} - async notifyChange() { - this.dirty = true; - await Promise.resolve(); - if (this.dirty) { - this.dirty = false; - this.notifyCB(); - } - } observe(value: T, parent?: any): T { if (value === null || typeof value !== "object" || value instanceof Date) { @@ -65,7 +57,7 @@ export class Observer { } self._updateRevNumber(target); target[key] = newVal; - self.notifyChange(); + self.notifyCB(); } return true; }, @@ -73,7 +65,7 @@ export class Observer { if (key in target) { delete target[key]; self._updateRevNumber(target); - self.notifyChange(); + self.notifyCB(); } return true; } diff --git a/src/hooks.ts b/src/hooks.ts index c572742e..99188b89 100644 --- a/src/hooks.ts +++ b/src/hooks.ts @@ -24,19 +24,9 @@ import { Observer } from "./core/observer"; export function useState(state: T): T { const component: Component = Component.current!; const __owl__ = component.__owl__; - const renderFn = __owl__.renderFn; - let lastRenderRevNumber; - __owl__.renderFn = function(comp, params) { - lastRenderRevNumber = __owl__.observer!.rev; - return renderFn(comp, params); - }; if (!__owl__.observer) { __owl__.observer = new Observer(); - __owl__.observer.notifyCB = () => { - if (lastRenderRevNumber < __owl__.observer!.rev) { - component.render(); - } - }; + __owl__.observer.notifyCB = component.render.bind(component); } return __owl__.observer.observe(state); } diff --git a/tests/component/component.test.ts b/tests/component/component.test.ts index 36d005ea..55820e90 100644 --- a/tests/component/component.test.ts +++ b/tests/component/component.test.ts @@ -3400,10 +3400,6 @@ describe("async rendering", () => { }); test("change state and call manually render: no unnecessary rendering", async () => { - // when the state is changed, the component isn't notified directly (we wait - // for a microtask tick before calling 'render'), so it may happen that - // another rendering is done meanwhile, already using the new value of the - // state class Widget extends Component { static template = xml`
`; state = useState({ val: 1 }); diff --git a/tests/core/observer.test.ts b/tests/core/observer.test.ts index 9ecc0f8d..8644ff21 100644 --- a/tests/core/observer.test.ts +++ b/tests/core/observer.test.ts @@ -408,7 +408,7 @@ describe("observer", () => { obj.a = 111; obj.f = 222; await nextMicroTick(); - expect(observer.notifyCB).toBeCalledTimes(4); + expect(observer.notifyCB).toBeCalledTimes(5); }); test("throw error when state is mutated in object if allowMutation=false", async () => {