diff --git a/hooks/test/browser/combinations.test.js b/hooks/test/browser/combinations.test.js
index 02db5b74f2..028110e8b9 100644
--- a/hooks/test/browser/combinations.test.js
+++ b/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 (
+
+ );
+ }
+
+ 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 (
+
+
+ More info
+
+
a tooltip
+
+ );
+ }
+
+ act(() => {
+ render(, scratch);
+ });
+
+ expect(effectLog).to.deep.equal([
+ 'set ref P',
+ 'set ref DIV',
+ 'tooltip layout effect',
+ 'anchor layout effect',
+ 'tooltip effect',
+ 'anchor effect'
+ ]);
+ });
});
diff --git a/hooks/test/browser/errorBoundary.test.js b/hooks/test/browser/errorBoundary.test.js
index 9412ef421d..0e7de2f625 100644
--- a/hooks/test/browser/errorBoundary.test.js
+++ b/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('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 Test;
+ }
+
+ function Child({ children }) {
+ useLayoutEffect(goodEffect);
+ return children;
+ }
+
+ function App() {
+ const [err] = useErrorBoundary();
+ return err ? (
+ Error
+ ) : (
+
+
+
+ );
+ }
+
+ render(, scratch);
+ expect(scratch.innerHTML).to.equal('Test');
+ expect(badEffect).to.be.calledOnce;
+ expect(goodEffect).to.be.calledOnce;
+
+ throwErr = true;
+ render(, scratch);
+ rerender();
+ expect(scratch.innerHTML).to.equal('Error
');
+ expect(thrower).to.be.calledOnce;
+ expect(badEffect).to.be.calledOnce;
+ expect(goodEffect).to.be.calledOnce;
+ });
});
diff --git a/hooks/test/browser/useLayoutEffect.test.js b/hooks/test/browser/useLayoutEffect.test.js
index 5a5e75eb7b..f20fa1198a 100644
--- a/hooks/test/browser/useLayoutEffect.test.js
+++ b/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 (
+
+ );
+ };
+
+ act(() => {
+ render(, scratch);
+ });
+ expect(calls).to.deep.equal([
+ 'callback ref inner',
+ 'callback ref outer',
+ 'doing effect'
+ ]);
+ });
});
diff --git a/src/component.js b/src/component.js
index 13a69114e7..2270391120 100644
--- a/src/component.js
+++ b/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);
diff --git a/src/diff/children.js b/src/diff/children.js
index 2a59a0cc2a..90afbaf65e 100644
--- a/src/diff/children.js
+++ b/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} refQueue an array of elements needed to invoke refs
*/
export function diffChildren(
parentDom,
@@ -33,7 +34,8 @@ export function diffChildren(
excessDomChildren,
commitQueue,
oldDom,
- isHydrating
+ isHydrating,
+ refQueue
) {
let i,
j,
@@ -41,7 +43,6 @@ export function diffChildren(
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) {
diff --git a/src/diff/index.js b/src/diff/index.js
index 87c0ba21e4..2537a1b35b 100644
--- a/src/diff/index.js
+++ b/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} 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} commitQueue List of components
* which have callbacks to invoke in commitRoot
* @param {boolean} isHydrating Whether or not we are in hydration
+ * @param {Array} 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.
diff --git a/src/render.js b/src/render.js
index 3f92be680e..02901b041d 100644
--- a/src/render.js
+++ b/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);
}
/**
diff --git a/test/browser/refs.test.js b/test/browser/refs.test.js
index a5962ea500..a69b846239 100644
--- a/test/browser/refs.test.js
+++ b/test/browser/refs.test.js
@@ -1,5 +1,5 @@
import { setupRerender } from 'preact/test-utils';
-import { createElement, render, Component, createRef } from 'preact';
+import { createElement, render, Component, createRef, Fragment } from 'preact';
import { setupScratch, teardown } from '../_util/helpers';
/** @jsx createElement */
@@ -103,8 +103,8 @@ describe('refs', () => {
'called with H1',
'called with DIV',
'called with null',
- 'called with H1',
'called with null',
+ 'called with H1',
'called with DIV'
]);
});
@@ -543,4 +543,178 @@ describe('refs', () => {
expect(calls.length).to.equal(1);
expect(calls[0]).to.equal(scratch.firstChild.firstChild);
});
+
+ // Test for #4049
+ it('should first clean-up refs and after apply them', () => {
+ let calls = [];
+ let set;
+ class App extends Component {
+ constructor(props) {
+ super(props);
+ this.state = {
+ phase: 1
+ };
+ set = () => this.setState({ phase: 2 });
+ }
+
+ render(props, { phase }) {
+ return (
+
+ {phase === 1 ? (
+
+
+ r
+ ? calls.push('adding ref to two')
+ : calls.push('removing ref from two')
+ }
+ >
+ Element two
+
+
+ r
+ ? calls.push('adding ref to three')
+ : calls.push('removing ref from three')
+ }
+ >
+ Element three
+
+
+ ) : phase === 2 ? (
+
+
+ r
+ ? calls.push('adding ref to one')
+ : calls.push('removing ref from one')
+ }
+ >
+ Element one
+
+
+
+ r
+ ? calls.push('adding ref to two')
+ : calls.push('removing ref from two')
+ }
+ >
+ Element two
+
+
+ r
+ ? calls.push('adding ref to three')
+ : calls.push('removing ref from three')
+ }
+ >
+ Element three
+
+
+
+ ) : null}
+
+ );
+ }
+ }
+
+ render(, scratch);
+
+ expect(calls).to.deep.equal(['adding ref to two', 'adding ref to three']);
+ calls = [];
+
+ set();
+ rerender();
+ expect(calls).to.deep.equal([
+ 'removing ref from two',
+ 'adding ref to one',
+ 'adding ref to two',
+ 'adding ref to three'
+ ]);
+ });
+
+ it('should bind refs before componentDidMount', () => {
+ /** @type {import('preact').RefObject[]} */
+ const refs = [];
+
+ class Parent extends Component {
+ componentDidMount() {
+ // Child refs should be set
+ expect(refs.length).to.equal(2);
+ expect(refs[0].current.tagName).to.equal('SPAN');
+ expect(refs[1].current.tagName).to.equal('SPAN');
+ }
+
+ render(props) {
+ return props.children;
+ }
+ }
+
+ class Child extends Component {
+ constructor(props) {
+ super(props);
+
+ this.ref = createRef();
+ }
+
+ componentDidMount() {
+ // SPAN refs should be set
+ expect(this.ref.current.tagName).to.equal('SPAN');
+ expect(document.body.contains(this.ref.current)).to.equal(true);
+ refs.push(this.ref);
+ }
+
+ render() {
+ return Hello;
+ }
+ }
+
+ render(
+
+
+
+ ,
+ scratch
+ );
+
+ expect(scratch.innerHTML).to.equal('HelloHello');
+ expect(refs[0].current).to.equalNode(scratch.firstChild);
+ expect(refs[1].current).to.equalNode(scratch.lastChild);
+ });
+
+ it('should call refs after element is added to document on initial mount', () => {
+ const verifyRef = name => el =>
+ expect(document.body.contains(el), name).to.equal(true);
+
+ function App() {
+ return (
+
+ );
+ }
+
+ render(, scratch);
+ });
+
+ it('should call refs after element is added to document on update', () => {
+ const verifyRef = name => el =>
+ expect(document.body.contains(el), name).to.equal(true);
+
+ function App({ show = false }) {
+ return (
+
+ {show && (
+
+ Hello
+
+ )}
+
+ );
+ }
+
+ render(, scratch);
+ render(, scratch);
+ });
});