RFC: Remove HAS_HOOKS_STATE bit (#630)

* Thought exercise on sCU

* Account for hooks state and component state

* Add stub value

* Add changeset

* Add test

* Add test

.changeset/wet-jars-talk.md

@@ -0,0 +1,5 @@
+---
+"@preact/signals": patch
+---
+
+Change the way we deal with state settling hooks, when we know we are dealing with hooks that can settle their A -> B -> A state (and wind up at the same value). We should not verbatim rerender in our custom shouldComponentUpdate. Instead we should trust that hooks have handled their own state settling.

packages/preact/src/index.ts

@@ -320,38 +320,28 @@ Component.prototype.shouldComponentUpdate = function (
 	const updater = this._updater;
 	const hasSignals = updater && updater._sources !== undefined;
 
-	// let reason;
-	// if (!hasSignals && !hasComputeds.has(this)) {
-	// 	reason = "no signals or computeds";
-	// } else if (hasPendingUpdate.has(this)) {
-	// 	reason = "has pending update";
-	// } else if (hasHookState.has(this)) {
-	// 	reason = "has hook state";
-	// }
-	// if (reason) {
-	// 	if (!this) reason += " (`this` bug)";
-	// 	console.log("not optimizing", this?.constructor?.name, ": ", reason, {
-	// 		details: {
-	// 			hasSignals,
-	// 			hasComputeds: hasComputeds.has(this),
-	// 			hasPendingUpdate: hasPendingUpdate.has(this),
-	// 			hasHookState: hasHookState.has(this),
-	// 			deps: Array.from(updater._deps),
-	// 			updater,
-	// 		},
-	// 	});
-	// }
-
-	// if this component used no signals or computeds, update:
-	if (!hasSignals && !(this._updateFlags & HAS_COMPUTEDS)) return true;
-
-	// if there is a pending re-render triggered from Signals,
-	// or if there is hook or class state, update:
-	if (this._updateFlags & (HAS_PENDING_UPDATE | HAS_HOOK_STATE)) return true;
-
+	// If this is a component using state, rerender
 	// @ts-ignore
 	for (let i in state) return true;
 
+	if (this.__f || (typeof this.u == "boolean" && this.u === true)) {
+		const hasHooksState = this._updateFlags & HAS_HOOK_STATE;
+		// if this component used no signals or computeds and no hooks state, update:
+		if (!hasSignals && !hasHooksState && !(this._updateFlags & HAS_COMPUTEDS))
+			return true;
+
+		// if there is a pending re-render triggered from Signals,
+		// or if there is hooks state, update:
+		if (this._updateFlags & HAS_PENDING_UPDATE) return true;
+	} else {
+		// if this component used no signals or computeds, update:
+		if (!hasSignals && !(this._updateFlags & HAS_COMPUTEDS)) return true;
+
+		// if there is a pending re-render triggered from Signals,
+		// or if there is hooks state, update:
+		if (this._updateFlags & (HAS_PENDING_UPDATE | HAS_HOOK_STATE)) return true;
+	}
+
 	// if any non-Signal props changed, update:
 	for (let i in props) {
 		if (i !== "__source" && props[i] !== this.props[i]) return true;

packages/preact/src/internal.d.ts

@@ -19,6 +19,11 @@ export interface AugmentedElement extends HTMLElement {
 }
 
 export interface AugmentedComponent extends Component<any, any> {
+	//  hasScuFromHooks
+	// Preact 10.12 - Preact 10.25
+	u?: boolean;
+	// Preact 10.26 and onwards
+	__f?: boolean;
 	__v: VNode;
 	_updater?: Effect;
 	_updateFlags: number;

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));
@@ -829,4 +829,60 @@ describe("@preact/signals", () => {
 			expect(cleanup).to.have.been.calledOnceWith("foo", child);
 		});
 	});
+
+	// TODO: add test when we upgrade lockfile and Preact to latest
+	it.skip("Should take hooks-state settling in account", () => {
+		const renderSpy = sinon.spy();
+		const Context = createContext({
+			addModal: () => {},
+			removeModal: () => {},
+		});
+
+		function ModalProvider(props: any) {
+			let [modalCount, setModalCount] = useState(0);
+			renderSpy(modalCount);
+			let context = {
+				modalCount,
+				addModal() {
+					setModalCount(count => count + 1);
+				},
+				removeModal() {
+					setModalCount(count => count - 1);
+				},
+			};
+
+			return (
+				<Context.Provider value={context}>{props.children}</Context.Provider>
+			);
+		}
+
+		function useModal() {
+			let context = useContext(Context);
+			useEffect(() => {
+				context.addModal();
+				return () => {
+					context.removeModal();
+				};
+			}, [context]);
+		}
+
+		function Popover() {
+			useModal();
+			return <div>Popover</div>;
+		}
+
+		function App() {
+			return (
+				<ModalProvider>
+					<Popover />
+				</ModalProvider>
+			);
+		}
+
+		act(() => {
+			render(<App />, scratch);
+		});
+
+		expect(renderSpy).to.be.calledTwice;
+	});
 });