From 598c7fa6fad8785f7a2a425d8ed9d4a9c8e53779 Mon Sep 17 00:00:00 2001 From: Seva Zaikov Date: Sun, 9 Jun 2024 10:10:08 -0700 Subject: [PATCH] fix incorrect unmount behaviour with usevalue logic --- .../create-state/create-state.test.ts | 54 +++++++++++++++++- integration-tests/hooks.test.ts | 55 +++++++++++++++++++ src/create-element/create-element.ts | 2 +- src/create-element/parse-component.ts | 4 +- src/hooks/create-state.ts | 10 +++- 5 files changed, 120 insertions(+), 5 deletions(-) diff --git a/integration-tests/create-state/create-state.test.ts b/integration-tests/create-state/create-state.test.ts index 100abe8..64e9388 100644 --- a/integration-tests/create-state/create-state.test.ts +++ b/integration-tests/create-state/create-state.test.ts @@ -447,7 +447,7 @@ describe("createState", () => { expect(screen.getByTestId("container").textContent).toBe("new title"); }); - test("supports state changes correctly when conditional is return true/false/true", async () => { + test("supports state changes correctly when conditional returns true/false/true", async () => { let newValue = ""; const user = userEvent.setup(); function StateComponent() { @@ -574,4 +574,56 @@ describe("createState", () => { await user.click(btn); expect(screen.getByTestId("text").textContent).toBe("length is 17"); }); + + test.only("unsubscribes from updates if wasn't mounted", async () => { + const user = userEvent.setup(); + const valueState = createState(0); + function App() { + const showState = createState(true); + + return createElement("div", { + children: [ + createElement("button", { + "data-testid": "button", + onClick: () => showState.setValue(false), + }), + showState.useValue((shouldShow) => + shouldShow ? createElement(NestedComponent) : null + ), + ], + }); + } + + const spyFn = jest.fn(); + function NestedComponent() { + const x = createElement("div", { + children: [ + valueState.useValue((value) => { + spyFn(); + return `value is ${value}`; + }), + ], + }); + const showState = createState(false); + return createElement("div", { + children: [ + createElement("h1", { children: "nested component" }), + showState.useValue((shouldShow) => (shouldShow ? x : null)), + ], + }); + } + + attachComponent({ + htmlElement: document.body, + component: createElement(App), + }); + + valueState.setValue(1); + expect(spyFn).toHaveBeenCalledTimes(2); + + await user.click(screen.getByTestId("button")); + + // valueState.setValue(2); + // expect(spyFn).toHaveBeenCalledTimes(2); + }); }); diff --git a/integration-tests/hooks.test.ts b/integration-tests/hooks.test.ts index c21fa39..7b5613b 100644 --- a/integration-tests/hooks.test.ts +++ b/integration-tests/hooks.test.ts @@ -360,4 +360,59 @@ describe("lifecycle hooks", () => { }); expect(unmountSpy).toHaveBeenCalledTimes(1); }); + + test("calls unmounted correct amount of times", async () => { + const user = userEvent.setup(); + const unmountSpy = jest.fn(); + function App() { + onUnmount(() => { + unmountSpy("unmounting app"); + }); + const showState = createState(true); + + return createElement("div", { + children: [ + createElement("button", { + "data-testid": "button", + onClick: () => showState.setValue(false), + }), + showState.useValue((shouldShow) => + shouldShow + ? createElement(NestedComponent) + : createElement("div", { children: "other" }) + ), + ], + }); + } + + function NestedComponent() { + onUnmount(() => { + unmountSpy("unmounting nested component"); + }); + const t = createElement("h1", { children: "nested component" }); + t._privateMethods._addUnmountHandler(() => { + unmountSpy("unmounting h1 nested component"); + }); + return createElement("div", { + children: [t], + }); + } + + const component = createElement(App); + const removeVelesTree = attachComponent({ + htmlElement: document.body, + component, + }); + + await user.click(screen.getByTestId("button")); + + expect(unmountSpy).toHaveBeenCalledTimes(2); + expect(unmountSpy.mock.calls[0][0]).toBe("unmounting h1 nested component"); + expect(unmountSpy.mock.calls[1][0]).toBe("unmounting nested component"); + + removeVelesTree(); + + expect(unmountSpy).toHaveBeenCalledTimes(3); + expect(unmountSpy.mock.calls[2][0]).toBe("unmounting app"); + }); }); diff --git a/src/create-element/create-element.ts b/src/create-element/create-element.ts index 5eb3aed..d5eafe0 100644 --- a/src/create-element/create-element.ts +++ b/src/create-element/create-element.ts @@ -35,7 +35,7 @@ function createElement( let unmountHandlers: Function[] = []; const callUnmountHandlers = () => { // `onUnmount` is logically better to be executed on children first - childComponents.forEach((childComponent) => { + velesNode.childComponents.forEach((childComponent) => { childComponent._privateMethods._callUnmountHandlers(); }); diff --git a/src/create-element/parse-component.ts b/src/create-element/parse-component.ts index efac1ed..9e5e23c 100644 --- a/src/create-element/parse-component.ts +++ b/src/create-element/parse-component.ts @@ -58,12 +58,14 @@ function parseComponent({ }); }, _callUnmountHandlers: () => { - componentUnmountCbs.forEach((cb) => cb()); // this should trigger recursive checks, whether it is a VelesNode or VelesComponent // string Nodes don't have lifecycle handlers if ("_privateMethods" in velesComponent.tree) { velesComponent.tree._privateMethods._callUnmountHandlers(); } + + // we execute own unmount callbacks after children, so the order is reversed + componentUnmountCbs.forEach((cb) => cb()); }, }, }; diff --git a/src/hooks/create-state.ts b/src/hooks/create-state.ts index 2ed993f..8492839 100644 --- a/src/hooks/create-state.ts +++ b/src/hooks/create-state.ts @@ -137,6 +137,8 @@ function createState( const returnednewNode = cb ? cb(newSelectedValue) + : newSelectedValue == undefined + ? "" : String(newSelectedValue); const newNode = !returnednewNode || typeof returnednewNode === "string" @@ -270,7 +272,7 @@ function createState( // callbacks parentVelesElement.childComponents = parentVelesElement.childComponents.map((childComponent) => - childComponent === node ? newNode : node + childComponent === node ? newNode : childComponent ); // we call unmount handlers right after we replace it node._privateMethods._callUnmountHandlers(); @@ -661,7 +663,11 @@ function createState( ): VelesElement | VelesComponent | VelesStringElement { // @ts-expect-error const selectedValue = selector ? selector(value) : (value as F); - const returnedNode = cb ? cb(selectedValue) : String(selectedValue); + const returnedNode = cb + ? cb(selectedValue) + : selectedValue == undefined + ? "" + : String(selectedValue); const node = !returnedNode || typeof returnedNode === "string" ? createTextElement(returnedNode as string)