Merge branch 'master' into patch-2

debug/src/devtools/custom.js

@@ -145,7 +145,7 @@ export function getInstance(vnode) {
 	if (isRoot(vnode)) {
 		// Edge case: When the tree only consists of components that have not rendered
 		// anything into the DOM we revert to using the vnode as instance.
-		return vnode._children.length > 0 && vnode._children[0]._dom!=null
+		return vnode._children.length > 0 && vnode._children[0]!=null && vnode._children[0]._dom!=null
 			? /** @type {import('../internal').PreactElement | null} */
 			(vnode._children[0]._dom.parentNode)
 			: vnode;

debug/test/browser/devtools.test.js

@@ -681,14 +681,14 @@ describe('devtools', () => {
 			rerender();
 			checkEventReferences(prev.concat(hook.log));
 
-			// We swap unkeyed children if the match by type. In this case we'll
+			// We swap unkeyed children if they match by type. In this case we'll
 			// use `<Foo>bar</Foo>` as the old child to diff against for
 			// `<Foo>foo</Foo>`. That's why `<Foo>bar</Foo>` needs to be remounted.
 			expect(serialize(hook.log)).to.deep.equal([
-				{ type: 'update', component: 'Foo' },
-				{ type: 'mount', component: '#text: bar' },
+				{ type: 'mount', component: '#text: foo' },
 				{ type: 'mount', component: 'div' },
 				{ type: 'mount', component: 'Foo' },
+				{ type: 'update', component: 'Foo' },
 				{ type: 'update', component: 'App' },
 				{ type: 'rootCommitted', component: 'Fragment' }
 			]);

hooks/test/browser/combinations.test.js

@@ -1,5 +1,5 @@
-import { setupRerender } from 'preact/test-utils';
-import { createElement as h, render } from 'preact';
+import { setupRerender, act } from 'preact/test-utils';
+import { createElement as h, render, Component } from 'preact';
 import { setupScratch, teardown } from '../../../test/_util/helpers';
 import { useState, useReducer, useEffect, useLayoutEffect, useRef } from '../../src';
 import { scheduleEffectAssert } from '../_util/useEffectUtil';
@@ -174,4 +174,44 @@ describe('combinations', () => {
 			expect(effectCount).to.equal(1);
 		});
 	});
+
+	it('should not reuse functional components with hooks', () => {
+		let updater = { first: undefined, second: undefined };
+		function Foo(props) {
+			let [v, setter] = useState(0);
+			updater[props.id] = () => setter(++v);
+			return <div>{v}</div>;
+		}
+
+		let updateParent;
+		class App extends Component {
+			constructor(props) {
+				super(props);
+				this.state = { active: true };
+				updateParent = () => this.setState(p => ({ active: !p.active }));
+			}
+
+			render() {
+				return (
+					<div>
+						{this.state.active && <Foo id="first" />}
+						<Foo id="second" />
+					</div>
+				);
+			}
+		}
+
+		render(<App />, scratch);
+		act(() => updater.second());
+		expect(scratch.textContent).to.equal('01');
+
+		updateParent();
+		rerender();
+		expect(scratch.textContent).to.equal('1');
+
+		updateParent();
+		rerender();
+
+		expect(scratch.textContent).to.equal('01');
+	});
 });

src/diff/children.js

@@ -27,7 +27,7 @@ export function diffChildren(parentDom, newParentVNode, oldParentVNode, context,
 	let childVNode, i, j, p, index, oldVNode, newDom,
 		nextDom, sibDom, focus;
 
-	let newChildren = newParentVNode._children || toChildArray(newParentVNode.props.children, newParentVNode._children=[], coerceToVNode);
+	let newChildren = newParentVNode._children || toChildArray(newParentVNode.props.children, newParentVNode._children=[], coerceToVNode, true);
 	let oldChildren = oldParentVNode!=null && oldParentVNode!=EMPTY_OBJ && oldParentVNode._children || EMPTY_ARR;
 
 	let oldChildrenLength = oldChildren.length;
@@ -63,16 +63,19 @@ export function diffChildren(parentDom, newParentVNode, oldParentVNode, context,
 		// Check if we find a corresponding element in oldChildren and store the
 		// index where the element was found.
 		p = oldChildren[i];
-		if (p != null && (childVNode.key==null && p.key==null ? (childVNode.type === p.type) : (childVNode.key === p.key))) {
-			index = i;
-		}
-		else {
-			for (j=0; j<oldChildrenLength; j++) {
-				p = oldChildren[j];
-				if (p!=null) {
-					if (childVNode.key==null && p.key==null ? (childVNode.type === p.type) : (childVNode.key === p.key)) {
-						index = j;
-						break;
+
+		if (childVNode!=null) {
+			if (p===null || (p != null && (childVNode.key==null && p.key==null ? (childVNode.type === p.type) : (childVNode.key === p.key)))) {
+				index = i;
+			}
+			else {
+				for (j=0; j<oldChildrenLength; j++) {
+					p = oldChildren[j];
+					if (p!=null) {
+						if (childVNode.key==null && p.key==null ? (childVNode.type === p.type) : (childVNode.key === p.key)) {
+							index = j;
+							break;
+						}
 					}
 				}
 			}
@@ -83,7 +86,9 @@ export function diffChildren(parentDom, newParentVNode, oldParentVNode, context,
 		// element.
 		if (index!=null) {
 			oldVNode = oldChildren[index];
-			oldChildren[index] = null;
+			// We can't use `null` here because that is reserved for empty
+			// placeholders (holes)
+			oldChildren[index] = undefined;
 		}
 
 		nextDom = oldDom!=null && oldDom.nextSibling;
@@ -146,13 +151,19 @@ export function diffChildren(parentDom, newParentVNode, oldParentVNode, context,
  * @param {import('../index').ComponentChildren} children The unflattened
  * children of a virtual node
  * @param {Array<import('../internal').VNode | null>} [flattened] An flat array of children to modify
+ * @param {typeof import('../create-element').coerceToVNode} [map] Function that
+ * will be applied on each child if the `vnode` is not `null`
+ * @param {boolean} [keepHoles] wether to coerce `undefined` to `null` or not.
+ * This is needed for Components without children like `<Foo />`.
  */
-export function toChildArray(children, flattened, map) {
+export function toChildArray(children, flattened, map, keepHoles) {
 	if (flattened == null) flattened = [];
-	if (children==null || typeof children === 'boolean') {}
+	if (children==null || typeof children === 'boolean') {
+		if (keepHoles) flattened.push(null);
+	}
 	else if (Array.isArray(children)) {
 		for (let i=0; i < children.length; i++) {
-			toChildArray(children[i], flattened);
+			toChildArray(children[i], flattened, map, keepHoles);
 		}
 	}
 	else {

src/diff/index.js

@@ -50,7 +50,7 @@ export function diff(dom, parentDom, newVNode, oldVNode, context, isSvg, excessD
 			// we'll set `dom` to the correct value just a few lines later.
 			dom = null;
 
-			if (newVNode._children.length) {
+			if (newVNode._children.length && newVNode._children[0]!=null) {
 				dom = newVNode._children[0]._dom;
 
 				// If the last child is a Fragment, use _lastDomChild, else use _dom
@@ -137,7 +137,7 @@ export function diff(dom, parentDom, newVNode, oldVNode, context, isSvg, excessD
 
 			if (options.render) options.render(newVNode);
 
-			let prev = c._prevVNode;
+			let prev = c._prevVNode || null;
 			let vnode = c._prevVNode = coerceToVNode(c.render(c.props, c.state, c.context));
 			c._dirty = false;
 
@@ -347,7 +347,7 @@ export function unmount(vnode, ancestorComponent, skipRemove) {
 	}
 	else if (r = vnode._children) {
 		for (let i = 0; i < r.length; i++) {
-			unmount(r[i], ancestorComponent, skipRemove);
+			if (r[i]) unmount(r[i], ancestorComponent, skipRemove);
 		}
 	}
 

test/browser/components.test.js

@@ -500,9 +500,10 @@ describe('Components', () => {
 		Comp.prototype.componentWillMount.resetHistory();
 		bad.setState({ alt: true });
 		rerender();
-		expect(scratch.textContent, 'new component without key re-rendered').to.equal('D');
-		expect(Comp.prototype.componentWillMount).to.not.have.been.called;
-		expect(sideEffect).to.not.have.been.called;
+
+		expect(scratch.textContent, 'use null placeholders to detect new component is appended').to.equal('F');
+		expect(Comp.prototype.componentWillMount).to.be.calledOnce;
+		expect(sideEffect).to.be.calledOnce;
 	});
 
 	describe('array children', () => {

test/browser/focus.test.js

@@ -37,6 +37,8 @@ describe('focus', () => {
 	 * eqaul to the `input` parameter
 	 */
 	function validateFocus(input, message) {
+		// Check `nodeName` first to make cli output less spammy
+		expect(document.activeElement.nodeName).to.equal(input.nodeName, message);
 		expect(document.activeElement).to.equal(input, message);
 		expect(input.selectionStart).to.equal(2);
 		expect(input.selectionEnd).to.equal(5);
@@ -168,8 +170,6 @@ describe('focus', () => {
 
 		let input = focusInput();
 
-		input = focusInput();
-
 		render((
 			<List>
 				<ListItem>1</ListItem>

test/browser/keys.test.js

@@ -492,4 +492,30 @@ describe('keys', () => {
 		expect(Stateful1Ref).to.not.equal(Stateful1MovedRef);
 		expect(Stateful2Ref).to.not.equal(Stateful2MovedRef);
 	});
+
+	it('should treat undefined as a hole', () => {
+		let Bar = () => <div>bar</div>;
+
+		function Foo(props) {
+			let sibling;
+			if (props.condition) {
+				sibling = <Bar />;
+			}
+
+			return (
+				<div>
+					<div>Hello</div>
+					{sibling}
+				</div>
+			);
+		}
+
+		render(<Foo condition />, scratch);
+		clearLog();
+
+		render(<Foo />, scratch);
+		expect(getLog()).to.deep.equal([
+			'<div>bar.remove()'
+		]);
+	});
 });

test/browser/refs.test.js

@@ -348,4 +348,26 @@ describe('refs', () => {
 		render(<input type="text" ref={autoFocus} value="foo" />, scratch);
 		expect(input.value).to.equal('foo');
 	});
+
+	it('should correctly call child refs for un-keyed children on re-render', () => {
+		let el = null;
+		let ref = e => { el = e; };
+
+		class App extends Component {
+			render({ headerVisible }) {
+				return (
+					<div>
+						{headerVisible && <div>foo</div>}
+						<div ref={ref}>bar</div>
+					</div>
+				);
+			}
+		}
+
+		render(<App headerVisible />, scratch);
+		expect(el).to.not.be.equal(null);
+
+		render(<App />, scratch);
+		expect(el).to.not.be.equal(null);
+	});
 });

test/browser/render.test.js

@@ -907,4 +907,58 @@ describe('render()', () => {
 		// Without a fix it would render: `<div>foo</div><div></div>`
 		expect(scratch.innerHTML).to.equal('<div>foo</div>');
 	});
+
+	// see preact/#1327
+	it('should not reuse unkeyed components', () => {
+		class X extends Component {
+			constructor() {
+				super();
+				this.state = { i: 0 };
+			}
+
+			update() {
+				this.setState(prev => ({ i: prev.i + 1 }));
+			}
+
+			componentWillUnmount() {
+				clearTimeout(this.id);
+			}
+
+			render() {
+				return <div>{this.state.i}</div>;
+			}
+		}
+
+		let ref;
+		let updateApp;
+		class App extends Component {
+			constructor() {
+				super();
+				this.state = { i: 0 };
+				updateApp = () => this.setState(prev => ({ i: prev.i ^ 1 }));
+			}
+
+			render() {
+				return (
+					<div>
+						{this.state.i === 0 && <X />}
+						<X ref={node => ref = node} />
+					</div>
+				);
+			}
+		}
+
+		render(<App />, scratch);
+		expect(scratch.textContent).to.equal('00');
+
+		ref.update();
+		updateApp();
+		rerender();
+		expect(scratch.textContent).to.equal('1');
+
+		updateApp();
+		rerender();
+
+		expect(scratch.textContent).to.equal('01');
+	});
 });