fix: setting ref to null after updating it with new element (#4054)

hooks/test/browser/combinations.test.js

@@ -398,4 +398,83 @@ describe('combinations', () => {
 		});
 		expect(scratch.textContent).to.equal('2');
 	});
+
+	it.skip('parent and child refs should be set before all effects', () => {
+		const anchorId = 'anchor';
+		const tooltipId = 'tooltip';
+		const effectLog = [];
+
+		let useRef2 = sinon.spy(init => {
+			const realRef = useRef(init);
+			const ref = useRef(init);
+			Object.defineProperty(ref, 'current', {
+				get: () => realRef.current,
+				set: value => {
+					realRef.current = value;
+					effectLog.push('set ref ' + value?.tagName);
+				}
+			});
+			return ref;
+		});
+
+		function Tooltip({ anchorRef, children }) {
+			// For example, used to manually position the tooltip
+			const tooltipRef = useRef2(null);
+
+			useLayoutEffect(() => {
+				expect(anchorRef.current?.id).to.equal(anchorId);
+				expect(tooltipRef.current?.id).to.equal(tooltipId);
+				effectLog.push('tooltip layout effect');
+			}, [anchorRef, tooltipRef]);
+			useEffect(() => {
+				expect(anchorRef.current?.id).to.equal(anchorId);
+				expect(tooltipRef.current?.id).to.equal(tooltipId);
+				effectLog.push('tooltip effect');
+			}, [anchorRef, tooltipRef]);
+
+			return (
+				<div class="tooltip-wrapper">
+					<div id={tooltipId} ref={tooltipRef}>
+						{children}
+					</div>
+				</div>
+			);
+		}
+
+		function App() {
+			// For example, used to define what element to anchor the tooltip to
+			const anchorRef = useRef2(null);
+
+			useLayoutEffect(() => {
+				expect(anchorRef.current?.id).to.equal(anchorId);
+				effectLog.push('anchor layout effect');
+			}, [anchorRef]);
+			useEffect(() => {
+				expect(anchorRef.current?.id).to.equal(anchorId);
+				effectLog.push('anchor effect');
+			}, [anchorRef]);
+
+			return (
+				<div>
+					<p id={anchorId} ref={anchorRef}>
+						More info
+					</p>
+					<Tooltip anchorRef={anchorRef}>a tooltip</Tooltip>
+				</div>
+			);
+		}
+
+		act(() => {
+			render(<App />, scratch);
+		});
+
+		expect(effectLog).to.deep.equal([
+			'set ref P',
+			'set ref DIV',
+			'tooltip layout effect',
+			'anchor layout effect',
+			'tooltip effect',
+			'anchor effect'
+		]);
+	});
 });

hooks/test/browser/errorBoundary.test.js

@@ -1,6 +1,6 @@
 import { createElement, render } from 'preact';
 import { setupScratch, teardown } from '../../../test/_util/helpers';
-import { useErrorBoundary } from 'preact/hooks';
+import { useErrorBoundary, useLayoutEffect } from 'preact/hooks';
 import { setupRerender } from 'preact/test-utils';
 
 /** @jsx createElement */
@@ -107,4 +107,49 @@ describe('errorBoundary', () => {
 		expect(spy2).to.be.calledWith(error);
 		expect(scratch.innerHTML).to.equal('<p>Error</p>');
 	});
+
+	it('does not invoke old effects when a cleanup callback throws an error and is handled', () => {
+		let throwErr = false;
+		let thrower = sinon.spy(() => {
+			if (throwErr) {
+				throw new Error('test');
+			}
+		});
+		let badEffect = sinon.spy(() => thrower);
+		let goodEffect = sinon.spy();
+
+		function EffectThrowsError() {
+			useLayoutEffect(badEffect);
+			return <span>Test</span>;
+		}
+
+		function Child({ children }) {
+			useLayoutEffect(goodEffect);
+			return children;
+		}
+
+		function App() {
+			const [err] = useErrorBoundary();
+			return err ? (
+				<p>Error</p>
+			) : (
+				<Child>
+					<EffectThrowsError />
+				</Child>
+			);
+		}
+
+		render(<App />, scratch);
+		expect(scratch.innerHTML).to.equal('<span>Test</span>');
+		expect(badEffect).to.be.calledOnce;
+		expect(goodEffect).to.be.calledOnce;
+
+		throwErr = true;
+		render(<App />, scratch);
+		rerender();
+		expect(scratch.innerHTML).to.equal('<p>Error</p>');
+		expect(thrower).to.be.calledOnce;
+		expect(badEffect).to.be.calledOnce;
+		expect(goodEffect).to.be.calledOnce;
+	});
 });

hooks/test/browser/useLayoutEffect.test.js

@@ -476,4 +476,41 @@ describe('useLayoutEffect', () => {
 		expect(calls.length).to.equal(1);
 		expect(calls).to.deep.equal(['doing effecthi']);
 	});
+
+	it('should run layout affects after all refs are invoked', () => {
+		const calls = [];
+		const verifyRef = name => el => {
+			calls.push(name);
+			expect(document.body.contains(el), name).to.equal(true);
+		};
+
+		const App = () => {
+			const ref = useRef();
+			useLayoutEffect(() => {
+				expect(ref.current).to.equalNode(scratch.querySelector('p'));
+
+				calls.push('doing effect');
+				return () => {
+					calls.push('cleaning up');
+				};
+			});
+
+			return (
+				<div ref={verifyRef('callback ref outer')}>
+					<p ref={ref}>
+						<span ref={verifyRef('callback ref inner')}>Hi</span>
+					</p>
+				</div>
+			);
+		};
+
+		act(() => {
+			render(<App />, scratch);
+		});
+		expect(calls).to.deep.equal([
+			'callback ref inner',
+			'callback ref outer',
+			'doing effect'
+		]);
+	});
 });

src/component.js

@@ -125,7 +125,8 @@ function renderComponent(component) {
 		parentDom = component._parentDom;
 
 	if (parentDom) {
-		let commitQueue = [];
+		let commitQueue = [],
+			refQueue = [];
 		const oldVNode = assign({}, vnode);
 		oldVNode._original = vnode._original + 1;
 
@@ -138,9 +139,11 @@ function renderComponent(component) {
 			vnode._hydrating != null ? [oldDom] : null,
 			commitQueue,
 			oldDom == null ? getDomSibling(vnode) : oldDom,
-			vnode._hydrating
+			vnode._hydrating,
+			refQueue
 		);
-		commitRoot(commitQueue, vnode);
+
+		commitRoot(commitQueue, vnode, refQueue);
 
 		if (vnode._dom != oldDom) {
 			updateParentDomPointers(vnode);

src/diff/children.js

@@ -22,6 +22,7 @@ import { isArray } from '../util';
  * render (except when hydrating). Can be a sibling DOM element when diffing
  * Fragments that have siblings. In most cases, it starts out as `oldChildren[0]._dom`.
  * @param {boolean} isHydrating Whether or not we are in hydration
+ * @param {Array<any>} refQueue an array of elements needed to invoke refs
  */
 export function diffChildren(
 	parentDom,
@@ -33,15 +34,15 @@ export function diffChildren(
 	excessDomChildren,
 	commitQueue,
 	oldDom,
-	isHydrating
+	isHydrating,
+	refQueue
 ) {
 	let i,
 		j,
 		oldVNode,
 		childVNode,
 		newDom,
 		firstChildDom,
-		refs,
 		skew = 0;
 
 	// This is a compression of oldParentVNode!=null && oldParentVNode != EMPTY_OBJ && oldParentVNode._children || EMPTY_ARR
@@ -138,15 +139,17 @@ export function diffChildren(
 			excessDomChildren,
 			commitQueue,
 			oldDom,
-			isHydrating
+			isHydrating,
+			refQueue
 		);
 
 		newDom = childVNode._dom;
 
 		if ((j = childVNode.ref) && oldVNode.ref != j) {
-			if (!refs) refs = [];
-			if (oldVNode.ref) refs.push(oldVNode.ref, null, childVNode);
-			refs.push(j, childVNode._component || newDom, childVNode);
+			if (oldVNode.ref) {
+				applyRef(oldVNode.ref, null, childVNode);
+			}
+			refQueue.push(j, childVNode._component || newDom, childVNode);
 		}
 
 		if (newDom != null) {
@@ -243,13 +246,6 @@ export function diffChildren(
 			unmount(oldChildren[i], oldChildren[i]);
 		}
 	}
-
-	// Set refs only after unmount
-	if (refs) {
-		for (i = 0; i < refs.length; i++) {
-			applyRef(refs[i], refs[++i], refs[++i]);
-		}
-	}
 }
 
 function reorderChildren(childVNode, oldDom, parentDom) {

src/diff/index.js

@@ -20,7 +20,8 @@ import options from '../options';
  * element any new dom elements should be placed around. Likely `null` on first
  * render (except when hydrating). Can be a sibling DOM element when diffing
  * Fragments that have siblings. In most cases, it starts out as `oldChildren[0]._dom`.
- * @param {boolean} [isHydrating] Whether or not we are in hydration
+ * @param {boolean} isHydrating Whether or not we are in hydration
+ * @param {Array<any>} refQueue an array of elements needed to invoke refs
  */
 export function diff(
 	parentDom,
@@ -31,7 +32,8 @@ export function diff(
 	excessDomChildren,
 	commitQueue,
 	oldDom,
-	isHydrating
+	isHydrating,
+	refQueue
 ) {
 	let tmp,
 		newType = newVNode.type;
@@ -239,7 +241,8 @@ export function diff(
 				excessDomChildren,
 				commitQueue,
 				oldDom,
-				isHydrating
+				isHydrating,
+				refQueue
 			);
 
 			c.base = newVNode._dom;
@@ -269,7 +272,8 @@ export function diff(
 				isSvg,
 				excessDomChildren,
 				commitQueue,
-				isHydrating
+				isHydrating,
+				refQueue
 			);
 		}
 
@@ -293,7 +297,11 @@ export function diff(
  * which have callbacks to invoke in commitRoot
  * @param {import('../internal').VNode} root
  */
-export function commitRoot(commitQueue, root) {
+export function commitRoot(commitQueue, root, refQueue) {
+	for (let i = 0; i < refQueue.length; i++) {
+		applyRef(refQueue[i], refQueue[++i], refQueue[++i]);
+	}
+
 	if (options._commit) options._commit(root, commitQueue);
 
 	commitQueue.some(c => {
@@ -323,6 +331,7 @@ export function commitRoot(commitQueue, root) {
  * @param {Array<import('../internal').Component>} commitQueue List of components
  * which have callbacks to invoke in commitRoot
  * @param {boolean} isHydrating Whether or not we are in hydration
+ * @param {Array<any>} refQueue an array of elements needed to invoke refs
  * @returns {import('../internal').PreactElement}
  */
 function diffElementNodes(
@@ -333,7 +342,8 @@ function diffElementNodes(
 	isSvg,
 	excessDomChildren,
 	commitQueue,
-	isHydrating
+	isHydrating,
+	refQueue
 ) {
 	let oldProps = oldVNode.props;
 	let newProps = newVNode.props;
@@ -445,7 +455,8 @@ function diffElementNodes(
 				excessDomChildren
 					? excessDomChildren[0]
 					: oldVNode._children && getDomSibling(oldVNode, 0),
-				isHydrating
+				isHydrating,
+				refQueue
 			);
 
 			// Remove children that are not part of any vnode.

src/render.js

@@ -33,7 +33,8 @@ export function render(vnode, parentDom, replaceNode) {
 		createElement(Fragment, null, [vnode]);
 
 	// List of effects that need to be called after diffing.
-	let commitQueue = [];
+	let commitQueue = [],
+		refQueue = [];
 	diff(
 		parentDom,
 		// Determine the new vnode tree and store it on the DOM element on
@@ -55,11 +56,12 @@ export function render(vnode, parentDom, replaceNode) {
 			: oldVNode
 			? oldVNode._dom
 			: parentDom.firstChild,
-		isHydrating
+		isHydrating,
+		refQueue
 	);
 
 	// Flush all queued effects
-	commitRoot(commitQueue, vnode);
+	commitRoot(commitQueue, vnode, refQueue);
 }
 
 /**