From 32bf14a0f5845b49add3d52d6ba6cd2424b2cff9 Mon Sep 17 00:00:00 2001 From: Jovi De Croock Date: Fri, 29 Nov 2024 08:28:11 +0100 Subject: [PATCH 1/4] Delay executing state updates --- hooks/src/index.js | 129 ++++++++++---------------- hooks/src/internal.d.ts | 2 +- hooks/test/browser/useReducer.test.js | 13 ++- hooks/test/browser/useState.test.js | 12 ++- mangle.json | 1 + src/constants.js | 2 + src/diff/index.js | 21 ++++- src/index-5.d.ts | 1 + src/internal.d.ts | 2 + 9 files changed, 92 insertions(+), 91 deletions(-) diff --git a/hooks/src/index.js b/hooks/src/index.js index d6c434e400..f9af03c3e4 100644 --- a/hooks/src/index.js +++ b/hooks/src/index.js @@ -1,4 +1,5 @@ import { options as _options } from 'preact'; +import { SKIP_CHILDREN } from 'preact/src/constants'; /** @type {number} */ let currentIndex; @@ -55,10 +56,7 @@ options._render = vnode => { hooks._pendingEffects = []; currentComponent._renderCallbacks = []; hooks._list.forEach(hookItem => { - if (hookItem._nextValue) { - hookItem._value = hookItem._nextValue; - } - hookItem._pendingArgs = hookItem._nextValue = undefined; + hookItem._pendingArgs = undefined; }); } else { hooks._pendingEffects.forEach(invokeCleanup); @@ -181,96 +179,67 @@ export function useReducer(reducer, initialState, init) { const hookState = getHookState(currentIndex++, 2); hookState._reducer = reducer; if (!hookState._component) { + hookState._actions = []; hookState._value = [ !init ? invokeOrReturn(undefined, initialState) : init(initialState), action => { - const currentValue = hookState._nextValue - ? hookState._nextValue[0] - : hookState._value[0]; - const nextValue = hookState._reducer(currentValue, action); - - if (currentValue !== nextValue) { - hookState._nextValue = [nextValue, hookState._value[1]]; - hookState._component.setState({}); - } + hookState._actions.push(action); + hookState._component.setState({}); } ]; hookState._component = currentComponent; + } - if (!currentComponent._hasScuFromHooks) { - currentComponent._hasScuFromHooks = true; - let prevScu = currentComponent.shouldComponentUpdate; - const prevCWU = currentComponent.componentWillUpdate; - - // If we're dealing with a forced update `shouldComponentUpdate` will - // not be called. But we use that to update the hook values, so we - // need to call it. - currentComponent.componentWillUpdate = function (p, s, c) { - if (this._force) { - let tmp = prevScu; - // Clear to avoid other sCU hooks from being called - prevScu = undefined; - updateHookState(p, s, c); - prevScu = tmp; - } - - if (prevCWU) prevCWU.call(this, p, s, c); - }; - - // This SCU has the purpose of bailing out after repeated updates - // to stateful hooks. - // we store the next value in _nextValue[0] and keep doing that for all - // state setters, if we have next states and - // all next states within a component end up being equal to their original state - // we are safe to bail out for this specific component. - /** - * - * @type {import('./internal').Component["shouldComponentUpdate"]} - */ - // @ts-ignore - We don't use TS to downtranspile - // eslint-disable-next-line no-inner-declarations - function updateHookState(p, s, c) { - if (!hookState._component.__hooks) return true; - - /** @type {(x: import('./internal').HookState) => x is import('./internal').ReducerHookState} */ - const isStateHook = x => !!x._component; - const stateHooks = - hookState._component.__hooks._list.filter(isStateHook); - - const allHooksEmpty = stateHooks.every(x => !x._nextValue); - // When we have no updated hooks in the component we invoke the previous SCU or - // traverse the VDOM tree further. - if (allHooksEmpty) { - return prevScu ? prevScu.call(this, p, s, c) : true; - } - - // We check whether we have components with a nextValue set that - // have values that aren't equal to one another this pushes - // us to update further down the tree - let shouldUpdate = hookState._component.props !== p; - stateHooks.forEach(hookItem => { - if (hookItem._nextValue) { - const currentValue = hookItem._value[0]; - hookItem._value = hookItem._nextValue; - hookItem._nextValue = undefined; - if (currentValue !== hookItem._value[0]) shouldUpdate = true; - } - }); - - return prevScu - ? prevScu.call(this, p, s, c) || shouldUpdate - : shouldUpdate; - } + if (hookState._actions.length) { + const initialValue = hookState._value[0]; + hookState._actions.forEach(action => { + const value = hookState._value[0]; + hookState._value[0] = hookState._reducer(value, action); + }); - currentComponent.shouldComponentUpdate = updateHookState; - } + hookState._actions = []; + // This is incorrect, we set this shallowly based on the last state + // which means that if 2 useReducer calls have actions and the last one + // settles to an equal state we bail the previous one as well. This should + // be tackled all together in a post-render type of hook, that hook should + // also give us the ability to set SKIP_CHILDREN + hookState._updated = initialValue !== hookState._value[0]; + hookState._value = [hookState._value[0], hookState._value[1]]; + hookState._didExecute = true; } - return hookState._nextValue || hookState._value; + return hookState._value; } +const oldAfterRender = options._afterRender; +options._afterRender = (newVNode, oldVNode) => { + if (newVNode._component && newVNode._component.__hooks) { + const hooks = newVNode._component.__hooks._list; + const stateHooksThatExecuted = hooks.filter( + /** @type {(x: import('./internal').HookState) => x is import('./internal').ReducerHookState} */ + x => !!x._component && x._didExecute + ); + + if ( + stateHooksThatExecuted.length && + !stateHooksThatExecuted.some(x => x._updated) && + oldVNode.props === newVNode.props + ) { + newVNode._component.__hooks._pendingEffects = []; + newVNode._flags |= SKIP_CHILDREN; + } + + stateHooksThatExecuted.forEach(hook => { + hook._updated = undefined; + hook._didExecute = false; + }); + } + + if (oldAfterRender) oldAfterRender(newVNode); +}; + /** * @param {import('./internal').Effect} callback * @param {unknown[]} args diff --git a/hooks/src/internal.d.ts b/hooks/src/internal.d.ts index e828b198d7..45f9e2072d 100644 --- a/hooks/src/internal.d.ts +++ b/hooks/src/internal.d.ts @@ -80,7 +80,7 @@ export interface MemoHookState extends BaseHookState { export interface ReducerHookState extends BaseHookState { - _nextValue?: [S, StateUpdater]; + _actions: Array; _value?: [S, StateUpdater]; _component?: Component; _reducer?: Reducer; diff --git a/hooks/test/browser/useReducer.test.js b/hooks/test/browser/useReducer.test.js index 410805b766..00fb550136 100644 --- a/hooks/test/browser/useReducer.test.js +++ b/hooks/test/browser/useReducer.test.js @@ -194,11 +194,14 @@ describe('useReducer', () => { const badContextDefault = {}; const BadContext = createContext({}); - const Abstraction = ({ reducer, defaultState, children }) => ( - - {children} - - ); + const Abstraction = ({ reducer, defaultState, children }) => { + const state = useReducer(reducer, defaultState); + return ( + + {children} + + ); + }; const App = () => ( diff --git a/hooks/test/browser/useState.test.js b/hooks/test/browser/useState.test.js index ffa99e7902..8aa0be44e0 100644 --- a/hooks/test/browser/useState.test.js +++ b/hooks/test/browser/useState.test.js @@ -51,6 +51,8 @@ describe('useState', () => { expect(initState).to.be.calledOnce; }); + // TODO: check whether the delayed execution of + // useReducer should also be in useState it('does not rerender on equal state', () => { let lastState; let doSetState; @@ -69,12 +71,12 @@ describe('useState', () => { doSetState(0); rerender(); expect(lastState).to.equal(0); - expect(Comp).to.be.calledOnce; + expect(Comp).to.be.calledTwice; doSetState(() => 0); rerender(); expect(lastState).to.equal(0); - expect(Comp).to.be.calledOnce; + expect(Comp).to.be.calledThrice; }); it('rerenders when setting the state', () => { @@ -305,9 +307,11 @@ describe('useState', () => { let context = { modalCount, addModal() { + console.log('adding'); setModalCount(count => count + 1); }, removeModal() { + console.log('removing'); setModalCount(count => count - 1); } }; @@ -328,11 +332,13 @@ describe('useState', () => { } function Popover() { + console.log('POPOVER'); useModal(); return
Popover
; } function App() { + console.log('render app'); return ( @@ -344,7 +350,7 @@ describe('useState', () => { render(, scratch); }); - expect(renderSpy).to.be.calledTwice; + expect(renderSpy).to.be.calledThrice; }); // see preactjs/preact#3731 diff --git a/mangle.json b/mangle.json index 1d009545cb..8e001eaed5 100644 --- a/mangle.json +++ b/mangle.json @@ -70,6 +70,7 @@ "$_diff": "__b", "$_commit": "__c", "$_addHookName": "__a", + "$_afterRender": "__d", "$_render": "__r", "$_hook": "__h", "$_catchError": "__e", diff --git a/src/constants.js b/src/constants.js index 197783b081..cf221250de 100644 --- a/src/constants.js +++ b/src/constants.js @@ -7,6 +7,8 @@ export const INSERT_VNODE = 1 << 2; /** Indicates a VNode has been matched with another VNode in the diff */ export const MATCHED = 1 << 1; +export const SKIP_CHILDREN = 1 << 18; + /** Reset all mode flags */ export const RESET_MODE = ~(MODE_HYDRATE | MODE_SUSPENDED); diff --git a/src/diff/index.js b/src/diff/index.js index 6e139a6094..a61fd6d3b1 100644 --- a/src/diff/index.js +++ b/src/diff/index.js @@ -5,8 +5,9 @@ import { MODE_SUSPENDED, RESET_MODE, SVG_NAMESPACE, - UNDEFINED, - XHTML_NAMESPACE + XHTML_NAMESPACE, + SKIP_CHILDREN, + UNDEFINED } from '../constants'; import { BaseComponent, getDomSibling } from '../component'; import { Fragment } from '../create-element'; @@ -219,6 +220,7 @@ export function diff( c._force = false; let renderHook = options._render, + afterRender = options._afterRender, count = 0; if (isClassComponent) { c.state = c._nextState; @@ -228,6 +230,8 @@ export function diff( tmp = c.render(c.props, c.state, c.context); + if (afterRender) afterRender(newVNode, oldVNode); + for (let i = 0; i < c._stateCallbacks.length; i++) { c._renderCallbacks.push(c._stateCallbacks[i]); } @@ -239,6 +243,19 @@ export function diff( tmp = c.render(c.props, c.state, c.context); + if (afterRender) afterRender(newVNode, oldVNode); + + if (newVNode._flags & SKIP_CHILDREN) { + c._dirty = false; + c._renderCallbacks = []; + newVNode._dom = oldVNode._dom; + newVNode._children = oldVNode._children; + newVNode._children.some(vnode => { + if (vnode) vnode._parent = newVNode; + }); + break outer; + } + // Handle setState called in render, see #2553 c.state = c._nextState; } while (c._dirty && ++count < 25); diff --git a/src/index-5.d.ts b/src/index-5.d.ts index 7311010d71..b9a98e19b9 100644 --- a/src/index-5.d.ts +++ b/src/index-5.d.ts @@ -347,6 +347,7 @@ export interface Options { requestAnimationFrame?(callback: () => void): void; debounceRendering?(cb: () => void): void; useDebugValue?(value: string | number): void; + _afterRender?(newVNode: VNode, oldVNode: VNode): void; _addHookName?(name: string | number): void; __suspenseDidResolve?(vnode: VNode, cb: () => void): void; // __canSuspenseResolve?(vnode: VNode, cb: () => void): void; diff --git a/src/internal.d.ts b/src/internal.d.ts index 7733b0f279..28b0f6ff88 100644 --- a/src/internal.d.ts +++ b/src/internal.d.ts @@ -35,6 +35,8 @@ export interface Options extends preact.Options { _commit?(vnode: VNode, commitQueue: Component[]): void; /** Attach a hook that is invoked before a vnode has rendered. */ _render?(vnode: VNode): void; + /** Attach a hook that is invoked after a vnode has rendered. */ + _afterRender?(vnode: VNode, oldVNode: VNode): void; /** Attach a hook that is invoked before a hook's state is queried. */ _hook?(component: Component, index: number, type: HookType): void; /** Bypass effect execution. Currenty only used in devtools for hooks inspection */ From 667a39b8c6a10c62886096b27be8c59274bbb1b6 Mon Sep 17 00:00:00 2001 From: Jovi De Croock Date: Sat, 30 Nov 2024 06:27:26 +0100 Subject: [PATCH 2/4] Add test --- hooks/src/index.js | 9 +-- hooks/src/internal.d.ts | 7 +- hooks/test/browser/useState.test.js | 99 ++++++++++++++++++++++++++++- mangle.json | 3 +- 4 files changed, 108 insertions(+), 10 deletions(-) diff --git a/hooks/src/index.js b/hooks/src/index.js index f9af03c3e4..f8aca9848f 100644 --- a/hooks/src/index.js +++ b/hooks/src/index.js @@ -205,7 +205,7 @@ export function useReducer(reducer, initialState, init) { // settles to an equal state we bail the previous one as well. This should // be tackled all together in a post-render type of hook, that hook should // also give us the ability to set SKIP_CHILDREN - hookState._updated = initialValue !== hookState._value[0]; + hookState._didUpdate = initialValue !== hookState._value[0]; hookState._value = [hookState._value[0], hookState._value[1]]; hookState._didExecute = true; } @@ -219,12 +219,13 @@ options._afterRender = (newVNode, oldVNode) => { const hooks = newVNode._component.__hooks._list; const stateHooksThatExecuted = hooks.filter( /** @type {(x: import('./internal').HookState) => x is import('./internal').ReducerHookState} */ + // @ts-expect-error x => !!x._component && x._didExecute ); if ( stateHooksThatExecuted.length && - !stateHooksThatExecuted.some(x => x._updated) && + !stateHooksThatExecuted.some(x => x._didUpdate) && oldVNode.props === newVNode.props ) { newVNode._component.__hooks._pendingEffects = []; @@ -232,12 +233,12 @@ options._afterRender = (newVNode, oldVNode) => { } stateHooksThatExecuted.forEach(hook => { - hook._updated = undefined; + hook._didUpdate = undefined; hook._didExecute = false; }); } - if (oldAfterRender) oldAfterRender(newVNode); + if (oldAfterRender) oldAfterRender(newVNode, oldVNode); }; /** diff --git a/hooks/src/internal.d.ts b/hooks/src/internal.d.ts index 45f9e2072d..4a13ff0a8e 100644 --- a/hooks/src/internal.d.ts +++ b/hooks/src/internal.d.ts @@ -2,7 +2,7 @@ import { Options as PreactOptions, Component as PreactComponent, VNode as PreactVNode, - Context as PreactContext, + Context as PreactContext } from '../../src/internal'; import { Reducer, StateUpdater } from '.'; @@ -52,8 +52,6 @@ export type HookState = interface BaseHookState { _value?: unknown; - _nextValue?: undefined; - _pendingValue?: undefined; _args?: undefined; _pendingArgs?: undefined; _component?: undefined; @@ -72,7 +70,6 @@ export interface EffectHookState extends BaseHookState { export interface MemoHookState extends BaseHookState { _value?: T; - _pendingValue?: T; _args?: unknown[]; _pendingArgs?: unknown[]; _factory?: () => T; @@ -84,6 +81,8 @@ export interface ReducerHookState _value?: [S, StateUpdater]; _component?: Component; _reducer?: Reducer; + _didExecute?: boolean; + _didUpdate?: boolean; } export interface ContextHookState extends BaseHookState { diff --git a/hooks/test/browser/useState.test.js b/hooks/test/browser/useState.test.js index 8aa0be44e0..607b1ac06b 100644 --- a/hooks/test/browser/useState.test.js +++ b/hooks/test/browser/useState.test.js @@ -1,6 +1,13 @@ import { setupRerender, act } from 'preact/test-utils'; import { createElement, render, createContext, Component } from 'preact'; -import { useState, useContext, useEffect } from 'preact/hooks'; +import { + useState, + useContext, + useEffect, + useLayoutEffect, + useReducer, + useRef +} from 'preact/hooks'; import { setupScratch, teardown } from '../../../test/_util/helpers'; /** @jsx createElement */ @@ -419,4 +426,94 @@ describe('useState', () => { expect(renders).to.equal(2); }); }); + + it('Should capture the closure in the reducer', () => { + function createContext2() { + const context = createContext(); + + const ProviderOrig = context.Provider; + context.Provider = ({ value, children }) => { + const valueRef = useRef(value); + const contextValue = useRef(); + + if (!contextValue.current) { + contextValue.current = { + value: valueRef, + listener: null + }; + } + + useLayoutEffect(() => { + valueRef.current = value; + if (contextValue.current.listener) { + contextValue.current.listener([value]); + } + }, [value]); + return ( + {children} + ); + }; + + return context; + } + + function useContextSelector(context) { + const contextValue = useContext(context); + const { + value: { current: value } + } = contextValue; + const [state, dispatch] = useReducer( + () => { + return { + value + }; + }, + { + value + } + ); + useLayoutEffect(() => { + contextValue.listener = dispatch; + }, []); + return state.value; + } + + const context = createContext2(); + let set; + + function Child() { + const [count, setState] = useContextSelector(context); + const [c, setC] = useState(0); + set = () => { + setC(s => s + 1); + setState(s => s + 1); + }; + return ( +
+
Context count: {count}
+
Local count: {c}
+
+ ); + } + + // Render this + function App() { + const [state, setState] = useState(0); + return ( + + + + ); + } + + act(() => { + render(, scratch); + }); + expect(scratch.textContent).to.equal('Context count: 0Local count: 0'); + + act(() => { + set(); + }); + expect(scratch.textContent).to.equal('Context count: 1Local count: 1'); + }); }); diff --git a/mangle.json b/mangle.json index 8e001eaed5..fb1310e475 100644 --- a/mangle.json +++ b/mangle.json @@ -32,7 +32,8 @@ "$_list": "__", "$_pendingEffects": "__h", "$_value": "__", - "$_nextValue": "__N", + "$_didExecute": "__N", + "$_didUpdate": "__U", "$_original": "__v", "$_args": "__H", "$_factory": "__h", From a2d65742c9c07f3f9c9ee383ec4b3fb41f48dd69 Mon Sep 17 00:00:00 2001 From: Jovi De Croock Date: Sun, 1 Dec 2024 16:09:48 +0100 Subject: [PATCH 3/4] Update src/constants.js --- hooks/src/index.js | 18 ++++++++---------- src/constants.js | 2 +- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/hooks/src/index.js b/hooks/src/index.js index f8aca9848f..1be204f66f 100644 --- a/hooks/src/index.js +++ b/hooks/src/index.js @@ -1,5 +1,5 @@ import { options as _options } from 'preact'; -import { SKIP_CHILDREN } from 'preact/src/constants'; +import { SKIP_CHILDREN } from '../../src/constants'; /** @type {number} */ let currentIndex; @@ -25,6 +25,7 @@ let oldAfterDiff = options.diffed; let oldCommit = options._commit; let oldBeforeUnmount = options.unmount; let oldRoot = options._root; +let oldAfterRender = options._afterRender; const RAF_TIMEOUT = 100; let prevRaf; @@ -194,12 +195,10 @@ export function useReducer(reducer, initialState, init) { if (hookState._actions.length) { const initialValue = hookState._value[0]; - hookState._actions.forEach(action => { - const value = hookState._value[0]; - hookState._value[0] = hookState._reducer(value, action); + hookState._actions.some(action => { + hookState._value[0] = hookState._reducer(hookState._value[0], action); }); - hookState._actions = []; // This is incorrect, we set this shallowly based on the last state // which means that if 2 useReducer calls have actions and the last one // settles to an equal state we bail the previous one as well. This should @@ -208,19 +207,19 @@ export function useReducer(reducer, initialState, init) { hookState._didUpdate = initialValue !== hookState._value[0]; hookState._value = [hookState._value[0], hookState._value[1]]; hookState._didExecute = true; + hookState._actions = []; } return hookState._value; } -const oldAfterRender = options._afterRender; options._afterRender = (newVNode, oldVNode) => { if (newVNode._component && newVNode._component.__hooks) { const hooks = newVNode._component.__hooks._list; const stateHooksThatExecuted = hooks.filter( /** @type {(x: import('./internal').HookState) => x is import('./internal').ReducerHookState} */ // @ts-expect-error - x => !!x._component && x._didExecute + x => x._component && x._didExecute ); if ( @@ -232,9 +231,8 @@ options._afterRender = (newVNode, oldVNode) => { newVNode._flags |= SKIP_CHILDREN; } - stateHooksThatExecuted.forEach(hook => { - hook._didUpdate = undefined; - hook._didExecute = false; + stateHooksThatExecuted.some(hook => { + hook._didExecute = hook._didUpdate = false; }); } diff --git a/src/constants.js b/src/constants.js index cf221250de..54b10517b8 100644 --- a/src/constants.js +++ b/src/constants.js @@ -7,7 +7,7 @@ export const INSERT_VNODE = 1 << 2; /** Indicates a VNode has been matched with another VNode in the diff */ export const MATCHED = 1 << 1; -export const SKIP_CHILDREN = 1 << 18; +export const SKIP_CHILDREN = 1 << 3; /** Reset all mode flags */ export const RESET_MODE = ~(MODE_HYDRATE | MODE_SUSPENDED); From 17960ecb116ef619dcdda34fb01edc219e8516df Mon Sep 17 00:00:00 2001 From: Jovi De Croock Date: Mon, 2 Dec 2024 10:38:29 +0100 Subject: [PATCH 4/4] Apply suggestions from code review --- hooks/test/browser/useState.test.js | 6 ------ 1 file changed, 6 deletions(-) diff --git a/hooks/test/browser/useState.test.js b/hooks/test/browser/useState.test.js index 607b1ac06b..d9c25523c7 100644 --- a/hooks/test/browser/useState.test.js +++ b/hooks/test/browser/useState.test.js @@ -58,8 +58,6 @@ describe('useState', () => { expect(initState).to.be.calledOnce; }); - // TODO: check whether the delayed execution of - // useReducer should also be in useState it('does not rerender on equal state', () => { let lastState; let doSetState; @@ -314,11 +312,9 @@ describe('useState', () => { let context = { modalCount, addModal() { - console.log('adding'); setModalCount(count => count + 1); }, removeModal() { - console.log('removing'); setModalCount(count => count - 1); } }; @@ -339,13 +335,11 @@ describe('useState', () => { } function Popover() { - console.log('POPOVER'); useModal(); return
Popover
; } function App() { - console.log('render app'); return (