fix: Ensure that text effects get disposed (#647)

* fix: Ensure that text effects get disposed

* Create unlucky-impalas-grow.md

---------

Co-authored-by: Jovi De Croock <[email protected]>

.changeset/unlucky-impalas-grow.md

@@ -0,0 +1,5 @@
+---
+"@preact/signals": patch
+---
+
+Ensure that text effects get disposed

mangle.json

@@ -19,6 +19,7 @@
       }
     },
     "compress": {
+      "pure_getters": false,
       "conditionals": false,
       "loops": false,
       "sequences": false

packages/preact/src/index.ts

@@ -33,6 +33,17 @@ const HAS_PENDING_UPDATE = 1 << 0;
 const HAS_HOOK_STATE = 1 << 1;
 const HAS_COMPUTEDS = 1 << 2;
 
+let oldNotify: (this: Effect) => void,
+	effectsQueue: Array<Effect> = [],
+	domQueue: Array<Effect> = [];
+
+// Capture the original `Effect.prototype._notify` method so that we can install
+// custom `._notify`s for each different use-case but still call the original
+// implementation in the end. Dispose the temporary effect immediately afterwards.
+effect(function (this: Effect) {
+	oldNotify = this._notify;
+})();
+
 // Install a Preact options hook
 function hook<T extends OptionsTypes>(hookName: T, hookFn: HookFn<T>) {
 	// @ts-ignore-next-line private options hooks usage
@@ -79,7 +90,7 @@ function SignalValue(this: AugmentedComponent, { data }: { data: Signal }) {
 	const currentSignal = useSignal(data);
 	currentSignal.value = data;
 
-	const s = useMemo(() => {
+	const [isText, s] = useMemo(() => {
 		let self = this;
 		// mark the parent component as having computeds so it gets optimized
 		let v = this.__v;
@@ -90,38 +101,51 @@ function SignalValue(this: AugmentedComponent, { data }: { data: Signal }) {
 			}
 		}
 
-		const wrappedSignal = computed(function (this: Effect) {
-			let data = currentSignal.value;
-			let s = data.value;
+		const wrappedSignal = computed(() => {
+			let s = currentSignal.value.value;
 			return s === 0 ? 0 : s === true ? "" : s || "";
 		});
 
-		const isText = computed(
-			() => isValidElement(wrappedSignal.value) || this.base?.nodeType !== 3
-		);
-
-		this._updater!._callback = () => {
-			if (isValidElement(s.peek()) || this.base?.nodeType !== 3) {
-				this._updateFlags |= HAS_PENDING_UPDATE;
-				this.setState({});
-				return;
-			}
-			(this.base as Text).data = s.peek();
-		};
+		const isText = computed(() => !isValidElement(wrappedSignal.value));
 
-		effect(function (this: Effect) {
-			if (!oldNotify) oldNotify = this._notify;
+		// Update text nodes directly without rerendering when the new value
+		// is also text.
+		const dispose = effect(function (this: Effect) {
 			this._notify = notifyDomUpdates;
-			const val = wrappedSignal.value;
-			if (isText.value && self.base) {
-				(self.base as Text).data = val;
+
+			// Subscribe to wrappedSignal updates only when its values are text...
+			if (isText.value) {
+				// ...but regardless of `self.base`'s current value, as it can be
+				// undefined before mounting or a non-text node. In both of those cases
+				// the update gets handled by a full rerender.
+				const value = wrappedSignal.value;
+				if (self.base && self.base.nodeType === 3) {
+					(self.base as Text).data = value;
+				}
 			}
 		});
 
-		return wrappedSignal;
+		// Piggyback this._updater's disposal to ensure that the text updater effect
+		// above also gets disposed on unmount.
+		const oldDispose = this._updater!._dispose;
+		this._updater!._dispose = function () {
+			dispose();
+			oldDispose.call(this);
+		};
+
+		return [isText, wrappedSignal];
 	}, []);
 
-	return s.value;
+	// Rerender the component whenever `data.value` changes from a VNode
+	// to another VNode, from text to a VNode, or from a VNode to text.
+	// That is, everything else except text-to-text updates.
+	//
+	// This also ensures that the backing DOM node types gets updated to
+	// text nodes and back when needed.
+	//
+	// For text-to-text updates, `.peek()` is used to skip full rerenders,
+	// leaving them to the optimized path above.
+	return isText.value ? s.peek() : s.value;
 }
 SignalValue.displayName = "_st";
 
@@ -254,7 +278,6 @@ function createPropUpdater(
 			props = newProps;
 		},
 		_dispose: effect(function (this: Effect) {
-			if (!oldNotify) oldNotify = this._notify;
 			this._notify = notifyDomUpdates;
 			const value = changeSignal.value.value;
 			// If Preact just rendered this value, don't render it again:
@@ -365,10 +388,6 @@ export function useComputed<T>(compute: () => T) {
 	return useMemo(() => computed<T>(() => $compute.current()), []);
 }
 
-let oldNotify: (this: Effect) => void,
-	effectsQueue: Array<Effect> = [],
-	domQueue: Array<Effect> = [];
-
 const deferEffects =
 	typeof requestAnimationFrame === "undefined"
 		? setTimeout
@@ -416,7 +435,6 @@ export function useSignalEffect(cb: () => void | (() => void)) {
 
 	useEffect(() => {
 		return effect(function (this: Effect) {
-			if (!oldNotify) oldNotify = this._notify;
 			this._notify = notifyEffects;
 			return callback.current();
 		});

packages/preact/test/index.test.tsx

@@ -8,7 +8,7 @@ import {
 } from "@preact/signals";
 import type { ReadonlySignal } from "@preact/signals";
 import { createElement, createRef, render, createContext } from "preact";
-import type { ComponentChildren, FunctionComponent } from "preact";
+import type { ComponentChildren, FunctionComponent, VNode } from "preact";
 import { useContext, useEffect, useRef, useState } from "preact/hooks";
 import { setupRerender, act } from "preact/test-utils";
 
@@ -232,6 +232,61 @@ describe("@preact/signals", () => {
 			expect(text).to.be.an.instanceOf(HTMLSpanElement);
 			expect(text.textContent).to.equal("d");
 		});
+
+		describe("garbage collection", function () {
+			// Skip GC tests if window.gc/global.gc is not defined.
+			before(function () {
+				if (typeof gc === "undefined") {
+					this.skip();
+				}
+			});
+
+			it("should not hold on to references to signals and computeds after unmount", async () => {
+				const sig = signal("test");
+
+				let update: (content: VNode) => void;
+				function App() {
+					const [content, setContent] = useState(<div>{sig}</div>);
+					update = setContent;
+					return content;
+				}
+
+				render(<App />, scratch);
+				expect(scratch.firstChild!.firstChild!).to.have.property(
+					"data",
+					"test"
+				);
+
+				let ref: WeakRef<ReadonlySignal>;
+				(function () {
+					// Create a new computed inside a new IIFE scope, so that
+					// we can explicitly hold a _only_ weak reference to it.
+					const c = computed(() => sig.value + " computed");
+					ref = new WeakRef(c);
+
+					// Mount the new computed to App. Wrap it inside <span> so that `c`
+					// will get unmounted when we replace the spans with divs later.
+					act(() => update(<span>{c}</span>));
+				})();
+
+				expect(scratch.firstChild!.firstChild!).to.have.property(
+					"data",
+					"test computed"
+				);
+
+				act(() => update(<div>{sig}</div>));
+				expect(scratch.firstChild!.firstChild!).to.have.property(
+					"data",
+					"test"
+				);
+
+				// Ensure that the computed has a chance to get GC'd.
+				(gc as () => void)();
+				await sleep(0);
+				(gc as () => void)();
+				expect(ref.deref()).to.be.undefined;
+			});
+		});
 	});
 
 	describe("Component bindings", () => {