Merge branch 'restructure' into restructure-component-prep

follow-ups.md

@@ -4,6 +4,7 @@
 
 - `[MAJOR]` Deprecated `component.base`
 - `[MAJOR]` Remove `replaceNode`, [use this technique instead](https://gist.github.com/developit/f321a9ef092ad39f54f8d7c8f99eb29a))
+- `[MAJOR]` Removed select `<option>` fix for IE11, using select in IE11 will always require you to specify a `value` attribute
 
 ## Root node follow ups
 
@@ -23,7 +24,6 @@
 
 ## Backing Node follow ups
 
-- Revisit `prevDom` code path for null placeholders
 - Address many TODOs
 - Move refs to internal renderCallbacks
 - Rewrite rerender loop to operate on internals, not components
@@ -77,9 +77,6 @@
 
 ## Other
 
-- `[MAJOR]` Remove select `<option>` IE11 fix in diffChildren and tell users to
-  always specify a value attribute for `<option>`. History:
-  https://github.com/preactjs/preact/pull/1838
 - One possible implementation for effect queues: Internal nodes can have a local
   queue of effects for that node while a global queue contains the internal
   nodes that have effects.

src/diff/children.js

@@ -32,7 +32,7 @@ export function diffChildren(
 	commitQueue,
 	startDom
 ) {
-	let i, j, newDom, refs;
+	let i, newDom, refs;
 
 	/** @type {import('../internal').Internal} */
 	let childInternal;
@@ -56,47 +56,13 @@ export function diffChildren(
 			continue;
 		}
 
-		// Check if we find a corresponding element in oldChildren.
-		// If found, delete the array item by setting to `undefined`.
-		// We use `undefined`, as `null` is reserved for empty placeholders
-		// (holes).
-		childInternal = oldChildren[i];
+		childInternal = findMatchingInternal(
+			childVNode,
+			oldChildren,
+			i,
+			oldChildrenLength
+		);
 
-		if (typeof childVNode === 'string') {
-			// We never move Text nodes, so we only check for an in-place match:
-			if (childInternal && childInternal._flags & TYPE_TEXT) {
-				oldChildren[i] = undefined;
-			} else {
-				// We're looking for a Text node, but this wasn't one: ignore it
-				childInternal = undefined;
-			}
-		} else if (
-			childInternal === null ||
-			(childInternal &&
-				childVNode.key == childInternal.key &&
-				childVNode.type === childInternal.type)
-		) {
-			oldChildren[i] = undefined;
-		} else {
-			// Either oldVNode === undefined or oldChildrenLength > 0,
-			// so after this loop oldVNode == null or oldVNode is a valid value.
-			for (j = 0; j < oldChildrenLength; j++) {
-				childInternal = oldChildren[j];
-				// If childVNode is unkeyed, we only match similarly unkeyed nodes, otherwise we match by key.
-				// We always match by type (in either case).
-				if (
-					childInternal &&
-					childVNode.key == childInternal.key &&
-					childVNode.type === childInternal.type
-				) {
-					oldChildren[j] = undefined;
-					break;
-				}
-				childInternal = null;
-			}
-		}
-
-		let prevDom;
 		let oldVNodeRef;
 		let nextDomSibling;
 		if (childInternal == null) {
@@ -131,10 +97,6 @@ export function diffChildren(
 		} else {
 			oldVNodeRef = childInternal.ref;
 
-			// TODO: Figure out if there is a better way to handle the null
-			// placeholder's scenario this addresses
-			prevDom = childInternal._dom;
-
 			// Morph the old element into the new one, but don't append it to the dom yet
 			nextDomSibling = patch(
 				parentDom,
@@ -168,30 +130,13 @@ export function diffChildren(
 		} else if (
 			startDom &&
 			childInternal != null &&
-			prevDom == startDom &&
 			startDom.parentNode != parentDom
 		) {
 			// The above condition is to handle null placeholders. See test in placeholder.test.js:
 			// `efficiently replace null placeholders in parent rerenders`
 			startDom = nextDomSibling;
 		}
 
-		// Browsers will infer an option's `value` from `textContent` when
-		// no value is present. This essentially bypasses our code to set it
-		// later in `diff()`. It works fine in all browsers except for IE11
-		// where it breaks setting `select.value`. There it will be always set
-		// to an empty string. Re-applying an options value will fix that, so
-		// there are probably some internal data structures that aren't
-		// updated properly.
-		//
-		// To fix it we make sure to reset the inferred value, so that our own
-		// value check in `diff()` won't be skipped.
-		if (parentInternal.type === 'option') {
-			// @ts-ignore We have validated that the type of parentDOM is 'option'
-			// in the above check
-			parentDom.value = '';
-		}
-
 		newChildren[i] = childInternal;
 	}
 
@@ -226,6 +171,57 @@ export function diffChildren(
 	return startDom;
 }
 
+/**
+ * @param {import('../internal').VNode | string} childVNode
+ * @param {import('../internal').Internal[]} oldChildren
+ * @param {number} i
+ * @param {number} oldChildrenLength
+ * @returns {import('../internal').Internal}
+ */
+function findMatchingInternal(childVNode, oldChildren, i, oldChildrenLength) {
+	// Check if we find a corresponding element in oldChildren.
+	// If found, delete the array item by setting to `undefined`.
+	// We use `undefined`, as `null` is reserved for empty placeholders
+	// (holes).
+	let childInternal = oldChildren[i];
+
+	if (typeof childVNode === 'string') {
+		// We never move Text nodes, so we only check for an in-place match:
+		if (childInternal && childInternal._flags & TYPE_TEXT) {
+			oldChildren[i] = undefined;
+		} else {
+			// We're looking for a Text node, but this wasn't one: ignore it
+			childInternal = undefined;
+		}
+	} else if (
+		childInternal === null ||
+		(childInternal &&
+			childVNode.key == childInternal.key &&
+			childVNode.type === childInternal.type)
+	) {
+		oldChildren[i] = undefined;
+	} else {
+		// Either oldVNode === undefined or oldChildrenLength > 0,
+		// so after this loop oldVNode == null or oldVNode is a valid value.
+		for (let j = 0; j < oldChildrenLength; j++) {
+			childInternal = oldChildren[j];
+			// If childVNode is unkeyed, we only match similarly unkeyed nodes, otherwise we match by key.
+			// We always match by type (in either case).
+			if (
+				childInternal &&
+				childVNode.key == childInternal.key &&
+				childVNode.type === childInternal.type
+			) {
+				oldChildren[j] = undefined;
+				break;
+			}
+			childInternal = null;
+		}
+	}
+
+	return childInternal;
+}
+
 /**
  * @param {import('../internal').Internal} internal
  * @param {import('../internal').PreactElement} startDom

src/diff/patch.js

@@ -118,12 +118,6 @@ export function patch(parentDom, newVNode, internal, commitQueue, startDom) {
 		// Once we have successfully rendered the new VNode, copy it's ID over
 		internal._vnodeId = newVNode._vnodeId;
 	} catch (e) {
-		if (e.then) {
-			// If a promise was thrown, reorderChildren in case this component is
-			// being hidden or moved
-			reorderChildren(internal, startDom, parentDom);
-		}
-
 		// @TODO: assign a new VNode ID here? Or NaN?
 		// newVNode._vnodeId = 0;
 		internal._flags |= e.then ? MODE_SUSPENDED : MODE_ERRORED;

src/diff/unmount.js

@@ -14,11 +14,12 @@ import { applyRef } from './refs';
  * @param {import('../internal').Internal} internal The virtual node to unmount
  * @param {import('../internal').Internal} parentInternal The parent of the VNode that
  * initiated the unmount
- * @param {boolean} [skipRemove] Flag that indicates that a parent node of the
+ * @param {number} [skipRemove] Flag that indicates that a parent node of the
  * current element is already detached from the DOM.
  */
 export function unmount(internal, parentInternal, skipRemove) {
-	let r;
+	let r,
+		i = 0;
 	if (options.unmount) options.unmount(internal);
 	internal._flags |= MODE_UNMOUNTING;
 
@@ -27,15 +28,6 @@ export function unmount(internal, parentInternal, skipRemove) {
 			applyRef(r, null, parentInternal);
 	}
 
-	let dom;
-	if (!skipRemove && internal._flags & TYPE_DOM) {
-		skipRemove = (dom = internal._dom) != null;
-	} else if (internal._flags & TYPE_ROOT) {
-		skipRemove = false;
-	}
-
-	internal._dom = null;
-
 	// TODO: May need to consider a way to check that this component has
 	// successfully mounted as well (e.g. if it threw while mounting and a parent
 	// error boundary is rendering error UI and so is attempting to unmount this
@@ -49,10 +41,20 @@ export function unmount(internal, parentInternal, skipRemove) {
 	}
 
 	if ((r = internal._children)) {
-		for (let i = 0; i < r.length; i++) {
-			if (r[i]) unmount(r[i], parentInternal, skipRemove);
+		for (; i < r.length; i++) {
+			if (r[i]) {
+				unmount(
+					r[i],
+					parentInternal,
+					skipRemove ? ~internal._flags & TYPE_ROOT : internal._flags & TYPE_DOM
+				);
+			}
 		}
 	}
 
-	if (dom != null) removeNode(dom);
+	if (!skipRemove && internal._flags & TYPE_DOM) {
+		removeNode(internal._dom);
+	}
+
+	internal._dom = null;
 }

test/browser/render.test.js

@@ -1170,4 +1170,43 @@ describe('render()', () => {
 		render(<div tabindex={null} />, scratch);
 		expect(scratch.firstChild.tabIndex).to.equal(defaultValue);
 	});
+
+	it('should only remove the highest parent when unmounting a tree of DOM', () => {
+		render(
+			<ul>
+				<li>Hello</li>
+				<li>World</li>
+			</ul>,
+			scratch
+		);
+
+		clearLog();
+		render(null, scratch);
+
+		expect(getLog()).to.deep.equal(['<ul>HelloWorld.remove()']);
+	});
+
+	it('should only remove the highest parent when unmounting a tree with components', () => {
+		const List = props => props.children;
+		const Item = props => <li>{props.children}</li>;
+		render(
+			<ul>
+				<List>
+					<Item>Hello</Item>
+					<Item>World</Item>
+				</List>
+			</ul>,
+			scratch
+		);
+
+		const items = scratch.querySelectorAll('li');
+
+		clearLog();
+		render(null, scratch);
+
+		expect(getLog()).to.deep.equal(['<ul>HelloWorld.remove()']);
+
+		expect(items[0]).to.have.property('parentNode').that.should.exist;
+		expect(items[1]).to.have.property('parentNode').that.should.exist;
+	});
 });