Prevent prior scheduled effects from destroying the execution context (#869)

* Prevent prior scheduled effects from destroying the execution context

* Add test for signal-effect as well

Author: JoviDeCroock

.changeset/many-bears-call.md

@@ -0,0 +1,5 @@
+---
+"@preact/signals": patch
+---
+
+Prevent scheduled effects from highjacking the execution-context

packages/preact/src/index.ts

@@ -44,7 +44,11 @@ let finishUpdate: (() => void) | undefined;
 
 function setCurrentUpdater(updater?: Effect) {
 	// end tracking for the current update:
-	if (finishUpdate) finishUpdate();
+	if (finishUpdate) {
+		const finish = finishUpdate;
+		finishUpdate = undefined;
+		finish();
+	}
 	// start tracking the new update:
 	finishUpdate = updater && updater._start();
 }
@@ -151,6 +155,7 @@ hook(OptionsTypes.DIFF, (old, vnode) => {
 
 /** Set up Updater before rendering a component */
 hook(OptionsTypes.RENDER, (old, vnode) => {
+	old(vnode);
 	setCurrentUpdater();
 
 	let updater;
@@ -170,7 +175,6 @@ hook(OptionsTypes.RENDER, (old, vnode) => {
 
 	currentComponent = component;
 	setCurrentUpdater(updater);
-	old(vnode);
 });
 
 /** Finish current updater if a component errors */

packages/preact/test/index.test.tsx

@@ -9,7 +9,7 @@ import {
 import type { ReadonlySignal } from "@preact/signals";
 import { createElement, createRef, render, createContext } from "preact";
 import type { ComponentChildren, FunctionComponent } from "preact";
-import { useContext, useRef, useState } from "preact/hooks";
+import { useContext, useEffect, useRef, useState } from "preact/hooks";
 import { setupRerender, act } from "preact/test-utils";
 
 const sleep = (ms?: number) => new Promise(r => setTimeout(r, ms));
@@ -605,6 +605,157 @@ describe("@preact/signals", () => {
 	});
 
 	describe("hooks mixed with signals", () => {
+		it("should not have a stale currentComponent when an effect triggers a synchronous render", () => {
+			const scratch2 = document.createElement("div");
+			const sig = signal("foo");
+			let setState: (v: string) => void;
+
+			function Inner() {
+				return <p>{sig.value}</p>;
+			}
+
+			function App() {
+				const [state, _setState] = useState("initial");
+				setState = _setState;
+
+				useEffect(() => {
+					render(<Inner />, scratch2);
+				}, []);
+
+				// This causes a crash because currentComponent._updateFlags is gone
+				useComputed(() => {});
+
+				return (
+					<div>
+						{state} {sig.value}
+					</div>
+				);
+			}
+
+			render(<App />, scratch);
+			expect(scratch.innerHTML).to.equal("<div>initial foo</div>");
+
+			// Update state — triggers a re-render.
+			// options._render will flush the pending effect which calls
+			// render(<Inner />, scratch2). This synchronous render overwrites
+			// currentComponent via the nested RENDER/DIFFED hooks.
+			act(() => {
+				setState!("updated");
+			});
+			expect(scratch.innerHTML).to.equal("<div>updated foo</div>");
+			expect(scratch2.innerHTML).to.equal("<p>foo</p>");
+
+			act(() => {
+				sig.value = "bar";
+			});
+			expect(scratch.innerHTML).to.equal("<div>updated bar</div>");
+		});
+
+		it("should not have a stale currentComponent when a signal effect triggers a synchronous render", () => {
+			const scratch2 = document.createElement("div");
+			const sig = signal("foo");
+			let setState: (v: string) => void;
+
+			function Inner({ state }: { state: string }) {
+				return <p>{state}</p>;
+			}
+
+			function App() {
+				const [state, _setState] = useState("initial");
+				setState = _setState;
+
+				// useSignalEffect wraps useEffect, so the outer useEffect
+				// gets flushed during options._render. When it fires, it
+				// creates a signal effect() that immediately invokes the
+				// callback, doing a synchronous render to another root.
+				useSignalEffect(() => {
+					render(<Inner state={sig.value} />, scratch2);
+				});
+
+				// This causes a crash because currentComponent._updateFlags is gone
+				useComputed(() => {});
+
+				return (
+					<div>
+						{state} {sig.value}
+					</div>
+				);
+			}
+
+			render(<App />, scratch);
+			expect(scratch.innerHTML).to.equal("<div>initial foo</div>");
+
+			act(() => {
+				setState!("updated");
+			});
+			expect(scratch.innerHTML).to.equal("<div>updated foo</div>");
+			expect(scratch2.innerHTML).to.equal("<p>foo</p>");
+
+			act(() => {
+				sig.value = "bar";
+			});
+			expect(scratch.innerHTML).to.equal("<div>updated bar</div>");
+		});
+
+		it("should not have a stale currentComponent when an already-setup signal effect fires during render", () => {
+			const scratch2 = document.createElement("div");
+			const sig = signal("foo");
+			const dep = signal(0);
+			let setState: (v: string) => void;
+
+			function Inner() {
+				return <p>{dep.value}</p>;
+			}
+
+			function App() {
+				const [state, _setState] = useState("initial");
+				setState = _setState;
+
+				// Signal effect is set up on mount and tracks dep.
+				// When dep changes synchronously during a render flush,
+				// this fires immediately, doing a synchronous render.
+				useSignalEffect(() => {
+					dep.value;
+					render(<Inner />, scratch2);
+				});
+
+				// Regular useEffect that bumps dep on mount.
+				// When flushed during options._render (from a state-driven
+				// re-render), it changes dep, which fires the signal effect.
+				useEffect(() => {
+					dep.value = dep.peek() + 1;
+				}, []);
+
+				useComputed(() => {});
+
+				return (
+					<div>
+						{state} {sig.value}
+					</div>
+				);
+			}
+
+			render(<App />, scratch);
+			expect(scratch.innerHTML).to.equal("<div>initial foo</div>");
+
+			// The useSignalEffect's useEffect and the regular useEffect
+			// are both pending. When options._render flushes them:
+			// 1. useSignalEffect's useEffect fires → creates effect() →
+			//    immediately runs, reads dep (0), does sync render
+			// 2. useEffect fires → dep.value = 1 → signal effect fires
+			//    synchronously → sync render again → corrupts currentComponent
+			// 3. App renders → useComputed accesses stale currentComponent
+			act(() => {
+				setState!("updated");
+			});
+			expect(scratch.innerHTML).to.equal("<div>updated foo</div>");
+
+			act(() => {
+				sig.value = "bar";
+			});
+			expect(scratch.innerHTML).to.equal("<div>updated bar</div>");
+		});
+
 		it("signals should not stop context from propagating", () => {
 			const ctx = createContext({ test: "should-not-exist" });
 			let update: any;