diff --git a/doc/reference/component.md b/doc/reference/component.md index 6d8331f3..0fed57ae 100644 --- a/doc/reference/component.md +++ b/doc/reference/component.md @@ -451,5 +451,6 @@ console.log(status(component)); // logs either: // - 'new', if the component is new and has not been mounted yet // - 'mounted', if the component is currently mounted +// - 'cancelled', if the component has not been mounted yet but will be destroyed soon // - 'destroyed' if the component is currently destroyed ``` diff --git a/src/runtime/component_node.ts b/src/runtime/component_node.ts index 62128757..98f580a4 100644 --- a/src/runtime/component_node.ts +++ b/src/runtime/component_node.ts @@ -145,6 +145,9 @@ export class ComponentNode

implements VNode= STATUS.CANCELLED) { + return; + } let current = this.fiber; if (current && (current.root!.locked || (current as any).bdom === true)) { await Promise.resolve(); @@ -171,7 +174,7 @@ export class ComponentNode

implements VNode= STATUS.CANCELLED) { return; } // We only want to actually render the component if the following two @@ -190,6 +193,20 @@ export class ComponentNode

implements VNode = new Set(); constructor() { this.requestAnimationFrame = Scheduler.requestAnimationFrame; @@ -23,6 +25,13 @@ export class Scheduler { this.tasks.add(fiber.root!); } + scheduleDestroy(node: ComponentNode) { + this.cancelledNodes.add(node); + if (this.frame === 0) { + this.frame = this.requestAnimationFrame(() => this.processTasks()); + } + } + /** * Process all current tasks. This only applies to the fibers that are ready. * Other tasks are left unchanged. @@ -39,15 +48,23 @@ export class Scheduler { } if (this.frame === 0) { - this.frame = this.requestAnimationFrame(() => { - this.frame = 0; - this.tasks.forEach((fiber) => this.processFiber(fiber)); - for (let task of this.tasks) { - if (task.node.status === STATUS.DESTROYED) { - this.tasks.delete(task); - } - } - }); + this.frame = this.requestAnimationFrame(() => this.processTasks()); + } + } + + processTasks() { + this.frame = 0; + for (let node of this.cancelledNodes) { + node._destroy(); + } + this.cancelledNodes.clear(); + for (let task of this.tasks) { + this.processFiber(task); + } + for (let task of this.tasks) { + if (task.node.status === STATUS.DESTROYED) { + this.tasks.delete(task); + } } } diff --git a/src/runtime/status.ts b/src/runtime/status.ts index 9f48552c..e0d74c62 100644 --- a/src/runtime/status.ts +++ b/src/runtime/status.ts @@ -7,15 +7,20 @@ import type { Component } from "./component"; export const enum STATUS { NEW, MOUNTED, // is ready, and in DOM. It has a valid el + // component has been created, but has been replaced by a newer component before being mounted + // it is cancelled until the next animation frame where it will be destroyed + CANCELLED, DESTROYED, } -type STATUS_DESCR = "new" | "mounted" | "destroyed"; +type STATUS_DESCR = "new" | "mounted" | "cancelled" | "destroyed"; export function status(component: Component): STATUS_DESCR { switch (component.__owl__.status) { case STATUS.NEW: return "new"; + case STATUS.CANCELLED: + return "cancelled"; case STATUS.MOUNTED: return "mounted"; case STATUS.DESTROYED: diff --git a/tests/components/__snapshots__/concurrency.test.ts.snap b/tests/components/__snapshots__/concurrency.test.ts.snap index a430f6c2..a3c86491 100644 --- a/tests/components/__snapshots__/concurrency.test.ts.snap +++ b/tests/components/__snapshots__/concurrency.test.ts.snap @@ -212,6 +212,73 @@ exports[`changing state before first render does not trigger a render 1`] = ` }" `; +exports[`component destroyed just after render 1`] = ` +"function anonymous(app, bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, comment } = bdom; + const comp1 = app.createComponent(\`B\`, true, false, false, []); + + return function template(ctx, node, key = \\"\\") { + return comp1({}, key + \`__1\`, node, this, null); + } +}" +`; + +exports[`component destroyed just after render 2`] = ` +"function anonymous(app, bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, comment } = bdom; + + return function template(ctx, node, key = \\"\\") { + const b2 = text(\`B\`); + const b3 = text(ctx['state'].value); + return multi([b2, b3]); + } +}" +`; + +exports[`components are not destroyed between animation frame 1`] = ` +"function anonymous(app, bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, comment } = bdom; + const comp1 = app.createComponent(\`B\`, true, false, false, []); + + return function template(ctx, node, key = \\"\\") { + let b2,b3; + b2 = text(\`A\`); + if (ctx['state'].flag) { + b3 = comp1({}, key + \`__1\`, node, this, null); + } + return multi([b2, b3]); + } +}" +`; + +exports[`components are not destroyed between animation frame 2`] = ` +"function anonymous(app, bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, comment } = bdom; + const comp1 = app.createComponent(\`C\`, true, false, false, []); + + return function template(ctx, node, key = \\"\\") { + const b2 = text(\`B\`); + const b3 = comp1({}, key + \`__1\`, node, this, null); + return multi([b2, b3]); + } +}" +`; + +exports[`components are not destroyed between animation frame 3`] = ` +"function anonymous(app, bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, comment } = bdom; + + return function template(ctx, node, key = \\"\\") { + return text(\`C\`); + } +}" +`; + exports[`concurrent renderings scenario 1 1`] = ` "function anonymous(app, bdom, helpers ) { diff --git a/tests/components/concurrency.test.ts b/tests/components/concurrency.test.ts index c23ff6dc..62909302 100644 --- a/tests/components/concurrency.test.ts +++ b/tests/components/concurrency.test.ts @@ -115,13 +115,7 @@ test("destroying/recreating a subwidget with different props (if start is not ov await nextMicroTick(); expect(n).toBe(2); - expect([ - "Child:willDestroy", - "W:willRender", - "Child:setup", - "Child:willStart", - "W:rendered", - ]).toBeLogged(); + expect(["W:willRender", "Child:setup", "Child:willStart", "W:rendered"]).toBeLogged(); def.resolve(); await nextTick(); @@ -130,6 +124,7 @@ test("destroying/recreating a subwidget with different props (if start is not ov expect([ "Child:willRender", "Child:rendered", + "Child:willDestroy", "W:willPatch", "Child:mounted", "W:patched", @@ -178,13 +173,13 @@ test("destroying/recreating a subcomponent, other scenario", async () => { "Child:setup", "Child:willStart", "Parent:rendered", - "Child:willDestroy", "Parent:willRender", "Child:setup", "Child:willStart", "Parent:rendered", "Child:willRender", "Child:rendered", + "Child:willDestroy", "Parent:willPatch", "Child:mounted", "Parent:patched", @@ -251,13 +246,13 @@ test("creating two async components, scenario 1", async () => { await nextTick(); expect(fixture.innerHTML).toBe(""); expect([ - "ChildA:willDestroy", "Parent:willRender", "ChildA:setup", "ChildA:willStart", "ChildB:setup", "ChildB:willStart", "Parent:rendered", + "ChildA:willDestroy", ]).toBeLogged(); defB.resolve(); @@ -703,13 +698,13 @@ test("rendering component again in next microtick", async () => { "Child:setup", "Child:willStart", "Parent:rendered", - "Child:willDestroy", "Parent:willRender", "Child:setup", "Child:willStart", "Parent:rendered", "Child:willRender", "Child:rendered", + "Child:willDestroy", "Parent:willPatch", "Child:mounted", "Parent:patched", @@ -1732,9 +1727,9 @@ test("concurrent renderings scenario 10", async () => { expect(fixture.innerHTML).toBe("

"); expect([ "ComponentA:willRender", - "ComponentC:willDestroy", "ComponentB:willUpdateProps", "ComponentA:rendered", + "ComponentC:willDestroy", ]).toBeLogged(); defB.resolve(); @@ -2282,7 +2277,6 @@ test("concurrent renderings scenario 16", async () => { "D:setup", "D:willStart", "C:rendered", - "D:willDestroy", "B:willRender", "C:willUpdateProps", "B:rendered", @@ -2290,6 +2284,7 @@ test("concurrent renderings scenario 16", async () => { "D:setup", "D:willStart", "C:rendered", + "D:willDestroy", ]).toBeLogged(); // at this point, C rendering is still pending, and nothing should have been @@ -2997,11 +2992,11 @@ test("t-key on dom node having a component", async () => { expect(fixture.innerHTML).toBe("
3
"); expect([ - "Child (2):willDestroy", "Child (3):setup", "Child (3):willStart", "Child (3):willRender", "Child (3):rendered", + "Child (2):willDestroy", "Child (1):willUnmount", "Child (1):willDestroy", "Child (3):mounted", @@ -3055,11 +3050,11 @@ test("t-key on dynamic async component (toggler is never patched)", async () => expect(fixture.innerHTML).toBe("
3
"); expect([ - "Child (2):willDestroy", "Child (3):setup", "Child (3):willStart", "Child (3):willRender", "Child (3):rendered", + "Child (2):willDestroy", "Child (1):willUnmount", "Child (1):willDestroy", "Child (3):mounted", @@ -3114,11 +3109,11 @@ test("t-foreach with dynamic async component", async () => { expect(fixture.innerHTML).toBe("
3
"); expect([ - "Child (2):willDestroy", "Child (3):setup", "Child (3):willStart", "Child (3):willRender", "Child (3):rendered", + "Child (2):willDestroy", "Child (1):willUnmount", "Child (1):willDestroy", "Child (3):mounted", @@ -3801,7 +3796,7 @@ test("destroyed component causes other soon to be destroyed component to rerende static template = xml``; state = useState({ val: 0 }); setup() { - c = this; + c = c || this; useLogLifecycle(); } } @@ -3846,8 +3841,6 @@ test("destroyed component causes other soon to be destroyed component to rerende parent.state.valueB = 2; await nextTick(); expect([ - "B:willDestroy", - "C:willDestroy", "A:willRender", "B:setup", "B:willStart", @@ -3858,6 +3851,8 @@ test("destroyed component causes other soon to be destroyed component to rerende "B:rendered", "C:willRender", "C:rendered", + "B:willDestroy", + "C:willDestroy", "A:willPatch", "C:mounted", "B:mounted", @@ -4200,6 +4195,116 @@ test("delayed render is not cancelled by upcoming render", async () => { ]).toBeLogged(); }); +test("components are not destroyed between animation frame", async () => { + const def = makeDeferred(); + class C extends Component { + static template = xml`C`; + setup() { + useLogLifecycle(); + } + } + class B extends Component { + static template = xml`B`; + static components = { C }; + setup() { + useLogLifecycle(); + onWillStart(() => { + return def; + }); + } + } + class A extends Component { + static template = xml`A`; + static components = { B }; + + state = useState({ flag: false }); + setup() { + useLogLifecycle(); + } + } + const a = await mount(A, fixture); + expect(fixture.innerHTML).toBe("A"); + expect(["A:setup", "A:willStart", "A:willRender", "A:rendered", "A:mounted"]).toBeLogged(); + + // turn the flag on, this will render A and stops at B because of def + a.state.flag = true; + await nextTick(); + expect(["A:willRender", "B:setup", "B:willStart", "A:rendered"]).toBeLogged(); + + // force a render of A + // => owl will need to create a new B component + // => initial B component will be cancelled + a.render(); + await nextMicroTick(); + expect([ + // note that B is not destroyed here. It is cancelled instead + "A:willRender", + "B:setup", + "B:willStart", + "A:rendered", + ]).toBeLogged(); + + // resolve def, so B render is unblocked + def.resolve(); + await nextTick(); + expect([ + "B:willRender", + "C:setup", + "C:willStart", + "B:rendered", + "C:willRender", + "C:rendered", + // animation frame callback starts here + "B:willDestroy", // B is destroyed here + "A:willPatch", + "C:mounted", + "B:mounted", + "A:patched", + ]).toBeLogged(); +}); + +test("component destroyed just after render", async () => { + let stateB: any; + + class B extends Component { + static template = xml`B`; + state = useState({ value: 1 }); + setup() { + stateB = this.state; + useLogLifecycle(); + } + } + class A extends Component { + static template = xml``; + static components = { B }; + setup() { + useLogLifecycle(); + } + } + const a = await mount(A, fixture); + expect(fixture.innerHTML).toBe("B1"); + expect([ + "A:setup", + "A:willStart", + "A:willRender", + "B:setup", + "B:willStart", + "A:rendered", + "B:willRender", + "B:rendered", + "B:mounted", + "A:mounted", + ]).toBeLogged(); + + stateB!.value++; // force a render of B + await nextMicroTick(); // wait for B render to actually start + a.__owl__.app.destroy(); + expect(["A:willUnmount", "B:willUnmount", "B:willDestroy", "A:willDestroy"]).toBeLogged(); + await nextTick(); + // check that B was not rendered after being destroyed + expect([]).toBeLogged(); +}); + // test.skip("components with shouldUpdate=false", async () => { // const state = { p: 1, cc: 10 }; diff --git a/tests/components/error_handling.test.ts b/tests/components/error_handling.test.ts index e78230e3..a24d4f9e 100644 --- a/tests/components/error_handling.test.ts +++ b/tests/components/error_handling.test.ts @@ -1444,6 +1444,7 @@ describe("can catch errors", () => { "Parent:willPatch", "Child:willUnmount", "Child:willDestroy", + "Parent:patched", "Parent:willRender", "Parent:rendered", "Parent:willPatch", @@ -1506,12 +1507,15 @@ describe("can catch errors", () => { parent.state.hasChild = false; await nextTick(); expect([ + "Parent:willRender", + "Parent:rendered", "Child:willDestroy", "Parent:willRender", "Parent:rendered", - "Parent:willPatch", - "Parent:patched", ]).toBeLogged(); + expect(fixture.innerHTML).toBe("1"); + await nextTick(); + expect(["Parent:willPatch", "Parent:patched"]).toBeLogged(); expect(fixture.innerHTML).toBe("2"); }); });