Backport 10 16 (#4351)

* backport 4050

* backport 4032

* backport 4048

* backport 4047

* backport 4051

* backport 4010

* backport 4054

* change from main

* linting

* add notes

* unskip test

compat/server.browser.js

@@ -1,4 +1,11 @@
+import { renderToString } from 'preact-render-to-string';
+
 export {
 	renderToString,
 	renderToString as renderToStaticMarkup
 } from 'preact-render-to-string';
+
+export default {
+	renderToString,
+	renderToStaticMarkup: renderToString
+};

compat/server.mjs

@@ -1,4 +1,11 @@
+import { renderToString } from 'preact-render-to-string';
+
 export {
 	renderToString,
 	renderToString as renderToStaticMarkup
 } from 'preact-render-to-string';
+
+export default {
+	renderToString,
+	renderToStaticMarkup: renderToString
+};

follow-ups.md

@@ -23,6 +23,15 @@ PR's that weren't backported yet, do they work?
 - Make this work https://github.com/preactjs/preact/pull/3306
 - https://github.com/preactjs/preact/pull/3696
 
+## To backport
+
+- https://github.com/preactjs/preact/pull/4043
+
+## To consider
+
+- Ref cleanup functions
+- eliminate `forwardRef` completely
+
 ## Root node follow ups
 
 - Investigate if the return value of `createRoot()` can be re-used as a root Node...
@@ -42,7 +51,6 @@ PR's that weren't backported yet, do they work?
 ## Backing Node follow ups
 
 - Address many TODOs
-- Move refs to internal renderCallbacks
 - Consider converting Fragment (and other special nodes, e.g. root, Portal,
   context?) to be numbers instead of functions with special diff handling
 

hooks/test/browser/combinations.test.js

@@ -398,4 +398,83 @@ describe('combinations', () => {
 		});
 		expect(scratch.textContent).to.equal('2');
 	});
+
+	it('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 */
@@ -89,4 +89,49 @@ describe('errorBoundary', () => {
 		expect(spy2).to.be.calledOnce;
 		expect(spy2).to.be.calledWith(error);
 	});
+
+	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/diff/children.js

@@ -13,6 +13,7 @@ import { patch } from './patch';
 import { unmount } from './unmount';
 import { createInternal, getDomSibling } from '../tree';
 import { isArray } from '../util';
+import { refQueue } from './commit';
 
 /**
  * Update an internal with new children.
@@ -30,7 +31,6 @@ export function patchChildren(internal, children, parentDom) {
 	let skew = 0;
 	let i;
 
-	let refs;
 	/** @type {import('../internal').Internal} */
 	let childInternal;
 
@@ -73,10 +73,8 @@ export function patchChildren(internal, children, parentDom) {
 		if (mountingChild) {
 			childInternal = createInternal(childVNode, internal);
 
-			if (!refs) refs = [];
-
 			if (childVNode.ref) {
-				refs.push(childInternal, undefined);
+				refQueue.push(childInternal);
 				childInternal.ref = childVNode.ref;
 			}
 
@@ -85,8 +83,7 @@ export function patchChildren(internal, children, parentDom) {
 				childInternal,
 				childVNode,
 				parentDom,
-				getDomSibling(internal, skewedIndex),
-				refs
+				getDomSibling(internal, skewedIndex)
 			);
 		}
 		// If this node suspended during hydration, and no other flags are set:
@@ -95,19 +92,17 @@ export function patchChildren(internal, children, parentDom) {
 			(childInternal.flags & (MODE_HYDRATE | MODE_SUSPENDED)) ===
 			(MODE_HYDRATE | MODE_SUSPENDED)
 		) {
-			if (!refs) refs = [];
-
 			if (childVNode.ref) {
-				refs.push(childInternal, undefined);
+				refQueue.push(childInternal);
 				childInternal.ref = childVNode.ref;
 			}
 
 			// We are resuming the hydration of a VNode
 			mount(childInternal, childVNode, parentDom, childInternal.data);
 		} else {
 			if (childInternal.ref != childVNode.ref) {
-				if (!refs) refs = [];
-				refs.push(childInternal, childInternal.ref);
+				applyRef(childInternal.ref, null, internal);
+				refQueue.push(childInternal);
 				childInternal.ref = childVNode.ref;
 			}
 
@@ -175,16 +170,6 @@ export function patchChildren(internal, children, parentDom) {
 			}
 		}
 	}
-
-	// Set refs only after unmount
-	if (refs) {
-		for (i = 0; i < refs.length; i += 2) {
-			const internal = refs[i];
-			if (refs[i + 1]) applyRef(refs[i + 1], null, internal);
-			if (internal && internal.ref)
-				applyRef(internal.ref, internal._component || internal.data, internal);
-		}
-	}
 }
 
 /**

src/diff/commit.js

@@ -1,16 +1,29 @@
 import options from '../options';
+import { applyRef } from './refs';
 
 /**
  * A list of components with effects that need to be run at the end of the current render pass.
  * @type {import('../internal').CommitQueue}
  */
 export let commitQueue = [];
+export let refQueue = [];
 
 /**
  * @param {import('../internal').Internal} rootInternal
  */
 export function commitRoot(rootInternal) {
-	let currentQueue = commitQueue;
+	let currentQueue = refQueue;
+	refQueue = [];
+
+	for (let i = 0; i < currentQueue.length; i++) {
+		applyRef(
+			currentQueue[i].ref,
+			currentQueue[i]._component || currentQueue[i].data,
+			currentQueue[i]
+		);
+	}
+
+	currentQueue = commitQueue;
 	commitQueue = [];
 
 	if (options._commit) options._commit(rootInternal, currentQueue);