Commit pending hook state in options._render

`useReducer` installed two closures on every hook-using component: a
`shouldComponentUpdate` that both decided the bail-out and committed
`_nextValue` -> `_value`, and a `componentWillUpdate` that existed only to
run that same commit on the forced-update path, where core skips sCU. It did
so by temporarily nulling `prevScu` and re-entering the sCU, smuggling a
mutation through a predicate.

The commit belongs in neither. `options._render` runs immediately before
every render, forced or not, and already ran this exact loop for the
same-component re-render branch. Hoisting it out unconditionally makes
`componentWillUpdate` unnecessary and leaves sCU a pure predicate.

Clearing `_pendingArgs` unconditionally is safe because `options.diffed`
promotes them after every successful diff, so leftovers belong to a render
that never committed and this render recomputes them. On bail-out
`_nextValue` simply stays pending; the dispatcher already reads
`_nextValue[0]` when present.

hooks: 1387 -> 1331 B brotli (-4.0%). Also drops a closure per component
instance, and stops compat's UNSAFE_* accessor setter from running
`Object.defineProperty` on every hook component.

The forced-update path had no coverage, so add a test that drives it through
a context provider.

Author: JoviDeCroock

hooks/src/index.js

@@ -1,5 +1,4 @@
 import { options as _options } from 'preact';
-import { COMPONENT_FORCE } from '../../src/constants';
 
 const ObjectIs = Object.is;
 
@@ -58,20 +57,26 @@ options._render = vnode => {
 	const hooks = currentComponent.__hooks;
 	if (hooks) {
 		if (previousComponent === currentComponent) {
-			hooks._pendingEffects = [];
 			currentComponent._renderCallbacks = [];
-			hooks._list.some(hookItem => {
-				if (hookItem._nextValue) {
-					hookItem._value = hookItem._nextValue;
-				}
-				hookItem._pendingArgs = hookItem._nextValue = undefined;
-			});
 		} else {
 			hooks._pendingEffects.some(invokeCleanup);
 			hooks._pendingEffects.some(invokeEffect);
-			hooks._pendingEffects = [];
 			currentIndex = 0;
 		}
+		hooks._pendingEffects = [];
+
+		// Committing pending state here — right before every render, forced or
+		// not — is what lets `shouldComponentUpdate` below stay a pure predicate
+		// and lets us skip installing a `componentWillUpdate` entirely. Pending
+		// args are always stale by this point: `options.diffed` promotes them
+		// after each successful diff, so anything left over belongs to a render
+		// that never committed and this render recomputes it.
+		hooks._list.some(hookItem => {
+			if (hookItem._nextValue) {
+				hookItem._value = hookItem._nextValue;
+			}
+			hookItem._pendingArgs = hookItem._nextValue = undefined;
+		});
 	}
 	previousComponent = currentComponent;
 };
@@ -207,52 +212,30 @@ export function useReducer(reducer, initialState, init) {
 
 		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._bits & COMPONENT_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);
-			};
+			const prevScu = currentComponent.shouldComponentUpdate;
 
 			// 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;
+			// 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.
+			// Applying those next values is `options._render`'s job, so this only
+			// has to answer the question, never mutate.
+			currentComponent.shouldComponentUpdate = function (p, s, c) {
+				const hooks = this.__hooks;
+				if (!hooks) return 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 updatedHook = false;
-				let shouldUpdate = hookState._component.props !== p;
-				hookState._component.__hooks._list.some(hookItem => {
+				let shouldUpdate = this.props !== p;
+				hooks._list.some(hookItem => {
 					if (hookItem._nextValue) {
 						updatedHook = true;
-						const currentValue = hookItem._value[0];
-						hookItem._value = hookItem._nextValue;
-						hookItem._nextValue = undefined;
-						if (!ObjectIs(currentValue, hookItem._value[0]))
+						if (!ObjectIs(hookItem._value[0], hookItem._nextValue[0])) {
 							shouldUpdate = true;
+						}
 					}
 				});
 
@@ -262,9 +245,7 @@ export function useReducer(reducer, initialState, init) {
 				}
 
 				return !updatedHook || shouldUpdate;
-			}
-
-			currentComponent.shouldComponentUpdate = updateHookState;
+			};
 		}
 	}
 

hooks/test/browser/useState.test.jsx

@@ -537,4 +537,43 @@ describe('useState', () => {
 		});
 		expect(scratch.innerHTML).to.equal('<p>1-1</p>');
 	});
+
+	it('applies pending state on a forced update', () => {
+		const ctx = createContext(0);
+		let setValue, setCount;
+
+		function Provider({ children }) {
+			const [value, _setValue] = useState(0);
+			setValue = _setValue;
+			return <ctx.Provider value={value}>{children}</ctx.Provider>;
+		}
+
+		function Child() {
+			const value = useContext(ctx);
+			const [count, _setCount] = useState(0);
+			setCount = _setCount;
+			return (
+				<p>
+					{value}-{count}
+				</p>
+			);
+		}
+
+		render(
+			<Provider>
+				<Child />
+			</Provider>,
+			scratch
+		);
+		expect(scratch.innerHTML).to.equal('<p>0-0</p>');
+
+		act(() => {
+			// A context change force-updates its subscribers, which means Child
+			// renders with `shouldComponentUpdate` skipped entirely. Its own
+			// pending state still has to be applied by that render.
+			setValue(1);
+			setCount(1);
+		});
+		expect(scratch.innerHTML).to.equal('<p>1-1</p>');
+	});
 });