From f7f5d637ab0b0a70ff05be469c969944f2aa6e78 Mon Sep 17 00:00:00 2001 From: Seva Zaikov Date: Sun, 9 Jun 2024 15:31:43 -0700 Subject: [PATCH] do not track attributes value until mounted (#58) --- .../create-state/create-state.test.ts | 35 ------ .../create-state/use-attribute.test.ts | 109 ++++++++++++++++++ src/create-element/parse-component.ts | 5 +- src/create-state/index.ts | 21 +++- src/create-state/trigger-updates.ts | 32 +---- src/create-state/types.d.ts | 2 +- src/create-state/update-useattribute-value.ts | 43 +++++++ 7 files changed, 173 insertions(+), 74 deletions(-) create mode 100644 integration-tests/create-state/use-attribute.test.ts create mode 100644 src/create-state/update-useattribute-value.ts diff --git a/integration-tests/create-state/create-state.test.ts b/integration-tests/create-state/create-state.test.ts index 0ad61f3..271e6cf 100644 --- a/integration-tests/create-state/create-state.test.ts +++ b/integration-tests/create-state/create-state.test.ts @@ -276,41 +276,6 @@ describe("createState", () => { expect(unmountSpy).toHaveBeenCalledTimes(3); }); - test("useAttribute does not re-mount the component", async () => { - const user = userEvent.setup(); - const spyFn = jest.fn(); - function StateComponent() { - onUnmount(spyFn); - const valueState = createState(0); - return createElement("div", { - children: [ - createElement("button", { - "data-testvalue": valueState.useAttribute((value) => String(value)), - "data-testid": "button", - onClick: () => { - valueState.setValue((currentValue) => currentValue + 1); - }, - }), - ], - }); - } - - cleanup = attachComponent({ - htmlElement: document.body, - component: createElement(StateComponent), - }); - - const btn = screen.getByTestId("button"); - expect(btn).toHaveAttribute("data-testvalue", "0"); - - await user.click(btn); - expect(btn).toHaveAttribute("data-testvalue", "1"); - - await user.click(btn); - expect(btn).toHaveAttribute("data-testvalue", "2"); - expect(spyFn).not.toHaveBeenCalled(); - }); - test("supports strings as returned value in useValue", async () => { const user = userEvent.setup(); function StateComponent() { diff --git a/integration-tests/create-state/use-attribute.test.ts b/integration-tests/create-state/use-attribute.test.ts new file mode 100644 index 0000000..6a3a350 --- /dev/null +++ b/integration-tests/create-state/use-attribute.test.ts @@ -0,0 +1,109 @@ +import { screen } from "@testing-library/dom"; +import userEvent from "@testing-library/user-event"; + +import { + attachComponent, + createElement, + createState, + onUnmount, +} from "../../src"; + +import type { State } from "../../src"; + +describe("createState", () => { + let cleanup: Function | undefined; + + afterEach(() => { + cleanup?.(); + cleanup = undefined; + }); + + test("useAttribute does not re-mount the component", async () => { + const user = userEvent.setup(); + const spyFn = jest.fn(); + function StateComponent() { + onUnmount(spyFn); + const valueState = createState(0); + return createElement("div", { + children: [ + createElement("button", { + "data-testvalue": valueState.useAttribute((value) => String(value)), + "data-testid": "button", + onClick: () => { + valueState.setValue((currentValue) => currentValue + 1); + }, + }), + ], + }); + } + + cleanup = attachComponent({ + htmlElement: document.body, + component: createElement(StateComponent), + }); + + const btn = screen.getByTestId("button"); + expect(btn).toHaveAttribute("data-testvalue", "0"); + + await user.click(btn); + expect(btn).toHaveAttribute("data-testvalue", "1"); + + await user.click(btn); + expect(btn).toHaveAttribute("data-testvalue", "2"); + expect(spyFn).not.toHaveBeenCalled(); + }); + + test("does not track updates in useAttribute until mounted", async () => { + const user = userEvent.setup(); + const spyFn = jest.fn(); + + const valueState = createState("initialValue"); + function App() { + const showState = createState(false); + const content = createElement("div", { + "data-testid": "attributeTest", + "data-value": valueState.useAttribute((value) => { + spyFn(); + return value; + }), + }); + return createElement("div", { + children: [ + createElement("div", { + children: "whatever", + }), + createElement("button", { + "data-testid": "button", + onClick: () => showState.setValue((currentValue) => !currentValue), + }), + showState.useValue((shouldShow) => (shouldShow ? content : null)), + ], + }); + } + + cleanup = attachComponent({ + htmlElement: document.body, + component: createElement(App), + }); + + expect(spyFn).toHaveBeenCalledTimes(1); + valueState.setValue("newValue1"); + expect(spyFn).toHaveBeenCalledTimes(1); + + await user.click(screen.getByTestId("button")); + expect(spyFn).toHaveBeenCalledTimes(2); + expect(screen.getByTestId("attributeTest").getAttribute("data-value")).toBe( + "newValue1" + ); + valueState.setValue("newValue2"); + expect(spyFn).toHaveBeenCalledTimes(3); + expect(screen.getByTestId("attributeTest").getAttribute("data-value")).toBe( + "newValue2" + ); + + // remove the element again to see that subscriptions are correctly removed + await user.click(screen.getByTestId("button")); + valueState.setValue("newValue3"); + expect(spyFn).toHaveBeenCalledTimes(3); + }); +}); diff --git a/src/create-element/parse-component.ts b/src/create-element/parse-component.ts index e61b02c..df5fff9 100644 --- a/src/create-element/parse-component.ts +++ b/src/create-element/parse-component.ts @@ -3,7 +3,6 @@ import { createTextElement } from "./create-text-element"; import type { VelesComponent, - VelesStringElement, VelesElementProps, ComponentAPI, ComponentFunction, @@ -55,7 +54,7 @@ function parseComponent({ componentAPI.onUnmount(mountCbResult); } }); - // componentMountCbs = []; + componentMountCbs = []; }, _callUnmountHandlers: () => { // this should trigger recursive checks, whether it is a VelesNode or VelesComponent @@ -66,7 +65,7 @@ function parseComponent({ // we execute own unmount callbacks after children, so the order is reversed componentUnmountCbs.forEach((cb) => cb()); - // componentUnmountCbs = []; + componentUnmountCbs = []; }, }, }; diff --git a/src/create-state/index.ts b/src/create-state/index.ts index ec6ea36..9d75120 100644 --- a/src/create-state/index.ts +++ b/src/create-state/index.ts @@ -4,6 +4,7 @@ import { createElement } from "../create-element/create-element"; import { createTextElement } from "../create-element/create-text-element"; import { triggerUpdates } from "./trigger-updates"; import { updateUseValueSelector } from "./update-usevalue-selector-value"; +import { updateUseAttributeValue } from "./update-useattribute-value"; import type { VelesElement, @@ -237,6 +238,7 @@ function createState( // It should be a separate subscription. }, useAttribute: (cb?: (value: T) => any) => { + const originalValue = value; const attributeValue = cb ? cb(value) : value; const attributeHelper = ( @@ -254,12 +256,21 @@ function createState( attributeName, attributeValue, }; - trackers.trackingAttributes.push(trackingElement); - node._privateMethods._addUnmountHandler(() => { - trackers.trackingAttributes = trackers.trackingAttributes.filter( - (trackingAttribute) => trackingAttribute !== trackingElement - ); + node._privateMethods._addMountHandler(() => { + trackers.trackingAttributes.push(trackingElement); + + if (value !== originalValue) { + // since the `element` will be modified in place, we don't need to + // replace it in the array or anything + updateUseAttributeValue({ element: trackingElement, value }); + } + + node._privateMethods._addUnmountHandler(() => { + trackers.trackingAttributes = trackers.trackingAttributes.filter( + (trackingAttribute) => trackingAttribute !== trackingElement + ); + }); }); return attributeValue; diff --git a/src/create-state/trigger-updates.ts b/src/create-state/trigger-updates.ts index 19eac73..adc53e2 100644 --- a/src/create-state/trigger-updates.ts +++ b/src/create-state/trigger-updates.ts @@ -1,5 +1,6 @@ import { getComponentVelesNode, callMountHandlers, unique } from "../_utils"; import { updateUseValueSelector } from "./update-usevalue-selector-value"; +import { updateUseAttributeValue } from "./update-useattribute-value"; import type { VelesElement, VelesComponent } from "../types"; import type { @@ -35,36 +36,7 @@ function triggerUpdates({ // attributes // the HTML node does not change, so we don't need to modify the array trackers.trackingAttributes.forEach((element) => { - const { cb, htmlElement, attributeName, attributeValue } = element; - const newAttributeValue = cb ? cb(value) : value; - - if (typeof newAttributeValue === "boolean") { - if (newAttributeValue) { - htmlElement.setAttribute(attributeName, ""); - } else { - htmlElement.removeAttribute(attributeName); - } - } else if (attributeName.startsWith("on")) { - // if the value is the same, it is either not set - // or we received the same event handler - // either way, no need to do anything - if (attributeValue === newAttributeValue) { - return; - } - - const eventName = - attributeName[2].toLocaleLowerCase() + attributeName.slice(3); - if (attributeValue) { - htmlElement.removeEventListener(eventName, attributeValue); - } - if (newAttributeValue && typeof newAttributeValue === "function") { - htmlElement.addEventListener(eventName, newAttributeValue); - } - // not the best approach, but it should work as expected - element.attributeValue = newAttributeValue; - } else { - htmlElement.setAttribute(attributeName, newAttributeValue); - } + updateUseAttributeValue({ element, value }); }); // tracked values diff --git a/src/create-state/types.d.ts b/src/create-state/types.d.ts index a4eaecf..1afe071 100644 --- a/src/create-state/types.d.ts +++ b/src/create-state/types.d.ts @@ -84,7 +84,7 @@ export type TrackingSelectorElement = { node: VelesElement | VelesComponent | VelesStringElement; }; -type TrackingAttribute = { +export type TrackingAttribute = { cb?: Function; htmlElement: HTMLElement; attributeName: string; diff --git a/src/create-state/update-useattribute-value.ts b/src/create-state/update-useattribute-value.ts new file mode 100644 index 0000000..54f1a02 --- /dev/null +++ b/src/create-state/update-useattribute-value.ts @@ -0,0 +1,43 @@ +import type { TrackingAttribute } from "./types"; + +function updateUseAttributeValue({ + element, + value, +}: { + element: TrackingAttribute; + value: T; +}) { + const { cb, htmlElement, attributeName, attributeValue } = element; + const newAttributeValue = cb ? cb(value) : value; + + if (typeof newAttributeValue === "boolean") { + if (newAttributeValue) { + htmlElement.setAttribute(attributeName, ""); + } else { + htmlElement.removeAttribute(attributeName); + } + } else if (attributeName.startsWith("on")) { + // if the value is the same, it is either not set + // or we received the same event handler + // either way, no need to do anything + if (attributeValue === newAttributeValue) { + return; + } + + const eventName = + attributeName[2].toLocaleLowerCase() + attributeName.slice(3); + if (attributeValue) { + htmlElement.removeEventListener(eventName, attributeValue); + } + if (newAttributeValue && typeof newAttributeValue === "function") { + htmlElement.addEventListener(eventName, newAttributeValue); + } + // not the best approach, but it should work as expected + // basically, update the array value in-place + element.attributeValue = newAttributeValue; + } else { + htmlElement.setAttribute(attributeName, newAttributeValue); + } +} + +export { updateUseAttributeValue };