From 4b9144f7f13815013f78299dd487344d3750fd8f Mon Sep 17 00:00:00 2001 From: Jovi De Croock Date: Fri, 10 Jan 2025 18:44:03 +0100 Subject: [PATCH] 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 | 5 +++ packages/preact/src/index.ts | 48 ++++++++++-------------- packages/preact/src/internal.d.ts | 5 +++ packages/preact/test/index.test.tsx | 58 ++++++++++++++++++++++++++++- 4 files changed, 86 insertions(+), 30 deletions(-) create mode 100644 .changeset/wet-jars-talk.md diff --git a/.changeset/wet-jars-talk.md b/.changeset/wet-jars-talk.md new file mode 100644 index 00000000..7bbf9ccf --- /dev/null +++ b/.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. diff --git a/packages/preact/src/index.ts b/packages/preact/src/index.ts index cc24fd51..2dd9360c 100644 --- a/packages/preact/src/index.ts +++ b/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; diff --git a/packages/preact/src/internal.d.ts b/packages/preact/src/internal.d.ts index 7fe89290..19912ecf 100644 --- a/packages/preact/src/internal.d.ts +++ b/packages/preact/src/internal.d.ts @@ -19,6 +19,11 @@ export interface AugmentedElement extends HTMLElement { } export interface AugmentedComponent extends Component { + // hasScuFromHooks + // Preact 10.12 - Preact 10.25 + u?: boolean; + // Preact 10.26 and onwards + __f?: boolean; __v: VNode; _updater?: Effect; _updateFlags: number; diff --git a/packages/preact/test/index.test.tsx b/packages/preact/test/index.test.tsx index 4e26214f..57773a5b 100644 --- a/packages/preact/test/index.test.tsx +++ b/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 ( + {props.children} + ); + } + + function useModal() { + let context = useContext(Context); + useEffect(() => { + context.addModal(); + return () => { + context.removeModal(); + }; + }, [context]); + } + + function Popover() { + useModal(); + return
Popover
; + } + + function App() { + return ( + + + + ); + } + + act(() => { + render(, scratch); + }); + + expect(renderSpy).to.be.calledTwice; + }); });