From 779003e715fdb343133f26c7341c63d2d54606d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= Date: Mon, 29 Nov 2021 14:06:53 +0100 Subject: [PATCH] [REF] component: remove onDestroyed, implement onWillDestroy --- src/component/component_node.ts | 58 ++++++------------ src/component/fibers.ts | 10 ---- src/component/lifecycle_hooks.ts | 4 +- src/index.ts | 2 +- .../__snapshots__/lifecycle.test.ts.snap | 60 +++++++++---------- tests/components/concurrency.test.ts | 22 +++---- tests/components/lifecycle.test.ts | 32 +++++----- tests/components/t_component.test.ts | 2 +- tests/helpers.ts | 8 +-- tests/reactivity.test.ts | 2 +- 10 files changed, 83 insertions(+), 117 deletions(-) diff --git a/src/component/component_node.ts b/src/component/component_node.ts index bb777fab..0379a9f6 100644 --- a/src/component/component_node.ts +++ b/src/component/component_node.ts @@ -8,7 +8,6 @@ import { MountFiber, MountOptions, RootFiber, - __internal__destroyed, } from "./fibers"; import { handleError, fibersInError } from "./error_handling"; import { applyDefaultProps } from "./props_validation"; @@ -95,7 +94,7 @@ export class ComponentNode mounted: LifecycleHook[] = []; willPatch: LifecycleHook[] = []; patched: LifecycleHook[] = []; - destroyed: LifecycleHook[] = []; + willDestroy: LifecycleHook[] = []; constructor(C: T, props: any, app: App, parent?: ComponentNode) { currentNode = this; @@ -180,34 +179,27 @@ export class ComponentNode } destroy() { - if (this.status === STATUS.MOUNTED) { - callWillUnmount(this); + let shouldRemove = this.status === STATUS.MOUNTED; + this._destroy(); + if (shouldRemove) { this.bdom!.remove(); } - callDestroyed(this); + } - function callWillUnmount(node: ComponentNode) { - const component = node.component; - for (let cb of node.willUnmount) { - cb.call(component); - } - for (let child of Object.values(node.children)) { - if (child.status === STATUS.MOUNTED) { - callWillUnmount(child); - } - } - } - - function callDestroyed(node: ComponentNode) { - const component = node.component; - node.status = STATUS.DESTROYED; - for (let child of Object.values(node.children)) { - callDestroyed(child); - } - for (let cb of node.destroyed) { + _destroy() { + const component = this.component; + if (this.status === STATUS.MOUNTED) { + for (let cb of this.willUnmount) { cb.call(component); } } + for (let child of Object.values(this.children)) { + child._destroy(); + } + for (let cb of this.willDestroy) { + cb.call(component); + } + this.status = STATUS.DESTROYED; } async updateAndRender(props: any, parentFiber: Fiber) { @@ -260,26 +252,10 @@ export class ComponentNode } beforeRemove() { - visitRemovedNodes(this); + this._destroy(); } remove() { this.bdom!.remove(); } } - -function visitRemovedNodes(node: ComponentNode) { - if (node.status === STATUS.MOUNTED) { - const component = node.component; - for (let cb of node.willUnmount) { - cb.call(component); - } - } - for (let child of Object.values(node.children)) { - visitRemovedNodes(child); - } - node.status = STATUS.DESTROYED; - if (node.destroyed.length) { - __internal__destroyed.push(node); - } -} diff --git a/src/component/fibers.ts b/src/component/fibers.ts index 8ebdb8fe..64f6272c 100644 --- a/src/component/fibers.ts +++ b/src/component/fibers.ts @@ -126,14 +126,6 @@ export class RootFiber extends Fiber { node.bdom!.patch(this.bdom!, Object.keys(node.children).length > 0); this.appliedToDom = true; - // Step 3: calling all destroyed hooks - for (let node of __internal__destroyed) { - for (let cb of node.destroyed) { - cb(); - } - } - __internal__destroyed.length = 0; - // Step 4: calling all mounted lifecycle hooks let mountedFibers = this.mounted; while ((current = mountedFibers.pop())) { @@ -165,8 +157,6 @@ export class RootFiber extends Fiber { } } -export let __internal__destroyed: ComponentNode[] = []; - type Position = "first-child" | "last-child"; export interface MountOptions { diff --git a/src/component/lifecycle_hooks.ts b/src/component/lifecycle_hooks.ts index e8e20e4c..fae89207 100644 --- a/src/component/lifecycle_hooks.ts +++ b/src/component/lifecycle_hooks.ts @@ -35,9 +35,9 @@ export function onWillUnmount(fn: () => Promise | void | any) { node.willUnmount.unshift(fn); } -export function onDestroyed(fn: () => Promise | void | any) { +export function onWillDestroy(fn: () => Promise | void | any) { const node = getCurrent()!; - node.destroyed.push(fn); + node.willDestroy.push(fn); } export function onWillRender(fn: () => void | any) { diff --git a/src/index.ts b/src/index.ts index 3010bb2d..8e087c1e 100644 --- a/src/index.ts +++ b/src/index.ts @@ -68,7 +68,7 @@ export { onPatched, onWillRender, onRendered, - onDestroyed, + onWillDestroy, onError, } from "./component/lifecycle_hooks"; diff --git a/tests/components/__snapshots__/lifecycle.test.ts.snap b/tests/components/__snapshots__/lifecycle.test.ts.snap index 79f21e38..5d487ec0 100644 --- a/tests/components/__snapshots__/lifecycle.test.ts.snap +++ b/tests/components/__snapshots__/lifecycle.test.ts.snap @@ -107,36 +107,6 @@ exports[`lifecycle hooks component semantics 6`] = ` }" `; -exports[`lifecycle hooks components are unmounted and destroyed if no longer in DOM 1`] = ` -"function anonymous(bdom, helpers -) { - let { text, createBlock, list, multi, html, toggler, component } = bdom; - let { withDefault, getTemplate, prepareList, withKey, zero, call, callSlot, capture, isBoundary, shallowEqual, setContextValue, toNumber, safeOutput } = helpers; - - let block1 = createBlock(\`
\`); - - return function template(ctx, node, key = \\"\\") { - return block1(); - } -}" -`; - -exports[`lifecycle hooks components are unmounted and destroyed if no longer in DOM 2`] = ` -"function anonymous(bdom, helpers -) { - let { text, createBlock, list, multi, html, toggler, component } = bdom; - let { withDefault, getTemplate, prepareList, withKey, zero, call, callSlot, capture, isBoundary, shallowEqual, setContextValue, toNumber, safeOutput } = helpers; - - return function template(ctx, node, key = \\"\\") { - let b2; - if (ctx['state'].ok) { - b2 = component(\`Child\`, {}, key + \`__1\`, node, ctx); - } - return multi([b2]); - } -}" -`; - exports[`lifecycle hooks components are unmounted and destroyed if no longer in DOM, even after updateprops 1`] = ` "function anonymous(bdom, helpers ) { @@ -171,6 +141,36 @@ exports[`lifecycle hooks components are unmounted and destroyed if no longer in }" `; +exports[`lifecycle hooks components are unmounted destroyed if no longer in DOM 1`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component } = bdom; + let { withDefault, getTemplate, prepareList, withKey, zero, call, callSlot, capture, isBoundary, shallowEqual, setContextValue, toNumber, safeOutput } = helpers; + + let block1 = createBlock(\`
\`); + + return function template(ctx, node, key = \\"\\") { + return block1(); + } +}" +`; + +exports[`lifecycle hooks components are unmounted destroyed if no longer in DOM 2`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component } = bdom; + let { withDefault, getTemplate, prepareList, withKey, zero, call, callSlot, capture, isBoundary, shallowEqual, setContextValue, toNumber, safeOutput } = helpers; + + return function template(ctx, node, key = \\"\\") { + let b2; + if (ctx['state'].ok) { + b2 = component(\`Child\`, {}, key + \`__1\`, node, ctx); + } + return multi([b2]); + } +}" +`; + exports[`lifecycle hooks hooks are called in proper order in widget creation/destruction 1`] = ` "function anonymous(bdom, helpers ) { diff --git a/tests/components/concurrency.test.ts b/tests/components/concurrency.test.ts index d7f684dc..b6da373c 100644 --- a/tests/components/concurrency.test.ts +++ b/tests/components/concurrency.test.ts @@ -64,7 +64,7 @@ describe("async rendering", () => { def.resolve(); await nextTick(); expect(status(w)).toBe("destroyed"); - expect(["W:setup", "W:willStart", "W:destroyed"]).toBeLogged(); + expect(["W:setup", "W:willStart", "W:willDestroy"]).toBeLogged(); }); }); @@ -112,7 +112,7 @@ test("destroying/recreating a subwidget with different props (if start is not ov expect([ "W:willRender", - "Child:destroyed", + "Child:willDestroy", "Child:setup", "Child:willStart", "W:rendered", @@ -190,7 +190,7 @@ test("creating two async components, scenario 1", async () => { expect(fixture.innerHTML).toBe(""); expect([ "Parent:willRender", - "ChildA:destroyed", + "ChildA:willDestroy", "ChildA:setup", "ChildA:willStart", "ChildB:setup", @@ -583,8 +583,8 @@ test("properly behave when destroyed/unmounted while rendering ", async () => { "Parent:willPatch", "Child:willUnmount", "SubChild:willUnmount", - "SubChild:destroyed", - "Child:destroyed", + "SubChild:willDestroy", + "Child:willDestroy", "Parent:patched", ]).toBeLogged(); @@ -642,7 +642,7 @@ test("rendering component again in next microtick", async () => { "Child:willStart", "Parent:rendered", "Parent:willRender", - "Child:destroyed", + "Child:willDestroy", "Child:setup", "Child:willStart", "Parent:rendered", @@ -1694,7 +1694,7 @@ test("concurrent renderings scenario 10", async () => { expect(rendered).toBe(1); expect([ "ComponentB:willRender", - "ComponentC:destroyed", + "ComponentC:willDestroy", "ComponentC:setup", "ComponentC:willStart", "ComponentB:rendered", @@ -2249,7 +2249,7 @@ test.skip("concurrent renderings scenario 16", async () => { "C:willUpdateProps", "C:willRender", "C:rendered", - "D:destroyed", + "D:willDestroy", "D:setup", "D:willStart", "D:willRender", @@ -2324,7 +2324,7 @@ test("calling render in destroy", async () => { "B:willRender", "B:rendered", "B:willUnmount", - "B:destroyed", + "B:willDestroy", "B:mounted", "B:willRender", "B:rendered", @@ -2522,7 +2522,7 @@ test("two renderings initiated between willPatch and patched", async () => { "Panel:rendered", "Parent:willPatch", "Panel:willUnmount", - "Panel:destroyed", + "Panel:willDestroy", "Panel:mounted", "Parent:patched", ]).toBeLogged(); @@ -2535,7 +2535,7 @@ test("two renderings initiated between willPatch and patched", async () => { "Parent:rendered", "Parent:willPatch", "Panel:willUnmount", - "Panel:destroyed", + "Panel:willDestroy", "Parent:patched", ]).toBeLogged(); }); diff --git a/tests/components/lifecycle.test.ts b/tests/components/lifecycle.test.ts index 376c488c..d39a869c 100644 --- a/tests/components/lifecycle.test.ts +++ b/tests/components/lifecycle.test.ts @@ -313,7 +313,7 @@ describe("lifecycle hooks", () => { Object.freeze(steps); }); - test("components are unmounted and destroyed if no longer in DOM", async () => { + test("components are unmounted destroyed if no longer in DOM", async () => { let steps: string[] = []; class Child extends Component { @@ -410,7 +410,7 @@ describe("lifecycle hooks", () => { "Parent:rendered", "Parent:willPatch", "Child:willUnmount", - "Child:destroyed", + "Child:willDestroy", "Parent:patched", ]).toBeLogged(); }); @@ -450,8 +450,8 @@ describe("lifecycle hooks", () => { expect([ "Parent:willUnmount", "Child:willUnmount", - "Child:destroyed", - "Parent:destroyed", + "Child:willDestroy", + "Parent:willDestroy", ]).toBeLogged(); }); @@ -568,8 +568,8 @@ describe("lifecycle hooks", () => { expect([ "Parent:willUnmount", "Child:willUnmount", - "Child:destroyed", - "Parent:destroyed", + "Child:willDestroy", + "Parent:willDestroy", ]).toBeLogged(); }); @@ -631,9 +631,9 @@ describe("lifecycle hooks", () => { "Parent:willUnmount", "Child:willUnmount", "GrandChild:willUnmount", - "GrandChild:destroyed", - "Child:destroyed", - "Parent:destroyed", + "GrandChild:willDestroy", + "Child:willDestroy", + "Parent:willDestroy", ]).toBeLogged(); }); @@ -675,7 +675,7 @@ describe("lifecycle hooks", () => { // immediately destroy everything app.destroy(); await nextTick(); - expect(["Parent:willUnmount", "Parent:destroyed"]).toBeLogged(); + expect(["Parent:willUnmount", "Parent:willDestroy"]).toBeLogged(); }); test("lifecycle semantics, part 4", async () => { @@ -732,9 +732,9 @@ describe("lifecycle hooks", () => { app.destroy(); expect([ "Parent:willUnmount", - "GrandChild:destroyed", - "Child:destroyed", - "Parent:destroyed", + "GrandChild:willDestroy", + "Child:willDestroy", + "Parent:willDestroy", ]).toBeLogged(); }); @@ -776,7 +776,7 @@ describe("lifecycle hooks", () => { "Parent:rendered", "Parent:willPatch", "Child:willUnmount", - "Child:destroyed", + "Child:willDestroy", "Parent:patched", ]).toBeLogged(); }); @@ -1016,7 +1016,7 @@ describe("lifecycle hooks", () => { "C:willPatch", "D:willPatch", "E:willUnmount", - "E:destroyed", + "E:willDestroy", "F:mounted", "D:patched", "C:patched", @@ -1061,7 +1061,7 @@ describe("lifecycle hooks", () => { "Parent:rendered", "Parent:willPatch", "Child:willUnmount", - "Child:destroyed", + "Child:willDestroy", "Parent:patched", ]).toBeLogged(); diff --git a/tests/components/t_component.test.ts b/tests/components/t_component.test.ts index 126af0cf..65d337a8 100644 --- a/tests/components/t_component.test.ts +++ b/tests/components/t_component.test.ts @@ -95,7 +95,7 @@ describe("t-component", () => { "ChildB:rendered", "Parent:willPatch", "ChildA:willUnmount", - "ChildA:destroyed", + "ChildA:willDestroy", "ChildB:mounted", "Parent:patched", ]).toBeLogged(); diff --git a/tests/helpers.ts b/tests/helpers.ts index 29555c08..fc665d3e 100644 --- a/tests/helpers.ts +++ b/tests/helpers.ts @@ -1,7 +1,7 @@ import { App, Component, - onDestroyed, + onWillDestroy, onMounted, onPatched, onWillRender, @@ -210,9 +210,9 @@ export function useLogLifecycle() { logStep(`${name}:willUnmount`); }); - onDestroyed(() => { - expect(name + ": " + status(component)).toBe(name + ": " + "destroyed"); - logStep(`${name}:destroyed`); + onWillDestroy(() => { + expect(status(component)).not.toBe("destroyed"); + logStep(`${name}:willDestroy`); }); } diff --git a/tests/reactivity.test.ts b/tests/reactivity.test.ts index 756f0de5..12dcc115 100644 --- a/tests/reactivity.test.ts +++ b/tests/reactivity.test.ts @@ -1492,7 +1492,7 @@ describe("Reactivity: useState", () => { "Parent:rendered", "Parent:willPatch", "Child:willUnmount", - "Child:destroyed", + "Child:willDestroy", "Parent:patched", ]).toBeLogged();