Fixes vdom issues with selecting insert before and rendering widgets after they have been destroyed (#921)

agubler · web-flow · commit 95fd0da8ce95 · 2018-05-01T17:03:45.000+01:00
* Re-work the insert before logic

* Ensure that the current dnode is used from map, destroy widgets sync and skip widgets that have been queued but are destroyed

* fix silly test typo
diff --git a/src/vdom.ts b/src/vdom.ts
@@ -529,40 +529,31 @@ function nodeAdded(dnode: InternalDNode, transitions: TransitionStrategy) {
 	}
 }
 
-function callOnDetach(dNodes: InternalDNode | InternalDNode[], parentInstance: DefaultWidgetBaseInterface): void {
-	dNodes = Array.isArray(dNodes) ? dNodes : [dNodes];
-	for (let i = 0; i < dNodes.length; i++) {
-		const dNode = dNodes[i];
-		if (isWNode(dNode)) {
-			if (dNode.rendered) {
-				callOnDetach(dNode.rendered, dNode.instance);
-			}
-			if (dNode.instance) {
-				const instanceData = widgetInstanceMap.get(dNode.instance)!;
-				instanceData.onDetach();
-			}
-		} else {
-			if (dNode.children) {
-				callOnDetach(dNode.children as InternalDNode[], parentInstance);
-			}
-		}
-	}
-}
-
 function nodeToRemove(dnode: InternalDNode, transitions: TransitionStrategy, projectionOptions: ProjectionOptions) {
 	if (isWNode(dnode)) {
 		const rendered = dnode.rendered || emptyArray;
+		if (dnode.instance) {
+			const instanceData = widgetInstanceMap.get(dnode.instance)!;
+			instanceData.onDetach();
+			instanceMap.delete(dnode.instance);
+		}
 		for (let i = 0; i < rendered.length; i++) {
 			nodeToRemove(rendered[i], transitions, projectionOptions);
 		}
 	} else {
 		const domNode = dnode.domNode;
 		const properties = dnode.properties;
+		if (dnode.children && dnode.children.length > 0) {
+			for (let i = 0; i < dnode.children.length; i++) {
+				nodeToRemove(dnode.children[i], transitions, projectionOptions);
+			}
+		}
 		const exitAnimation = properties.exitAnimation;
 		if (properties && exitAnimation) {
 			(domNode as HTMLElement).style.pointerEvents = 'none';
 			const removeDomNode = function() {
 				domNode && domNode.parentNode && domNode.parentNode.removeChild(domNode);
+				dnode.domNode = undefined;
 			};
 			if (typeof exitAnimation === 'function') {
 				exitAnimation(domNode as Element, removeDomNode, properties);
@@ -573,6 +564,7 @@ function nodeToRemove(dnode: InternalDNode, transitions: TransitionStrategy, pro
 			}
 		}
 		domNode && domNode.parentNode && domNode.parentNode.removeChild(domNode);
+		dnode.domNode = undefined;
 	}
 }
 
@@ -629,7 +621,7 @@ function updateChildren(
 	let i: number;
 	let textUpdated = false;
 	while (newIndex < newChildrenLength) {
-		const oldChild = oldIndex < oldChildrenLength ? oldChildren[oldIndex] : undefined;
+		let oldChild = oldIndex < oldChildrenLength ? oldChildren[oldIndex] : undefined;
 		const newChild = newChildren[newIndex];
 		if (isVNode(newChild) && typeof newChild.deferredPropertiesCallback === 'function') {
 			newChild.inserted = isVNode(oldChild) && oldChild.inserted;
@@ -644,27 +636,31 @@ function updateChildren(
 
 		const findOldIndex = findIndexOfChild(oldChildren, newChild, oldIndex + 1);
 		const addChild = () => {
-			let insertBefore: Element | Text | undefined = undefined;
+			let insertBeforeDomNode: Element | Text | undefined = undefined;
 			let child: InternalDNode = oldChildren[oldIndex];
 			if (child) {
 				let nextIndex = oldIndex + 1;
-				while (insertBefore === undefined) {
-					if (isWNode(child)) {
-						if (child.rendered) {
-							child = child.rendered[0];
-						} else if (oldChildren[nextIndex]) {
-							child = oldChildren[nextIndex];
-							nextIndex++;
-						} else {
-							break;
+				let insertBeforeChildren = [child];
+				while (insertBeforeChildren.length) {
+					const insertBefore = insertBeforeChildren.shift()!;
+					if (isWNode(insertBefore)) {
+						if (insertBefore.rendered) {
+							insertBeforeChildren.push(...insertBefore.rendered);
 						}
 					} else {
-						insertBefore = child.domNode;
+						if (insertBefore.domNode) {
+							insertBeforeDomNode = insertBefore.domNode;
+							break;
+						}
+					}
+					if (insertBeforeChildren.length === 0 && oldChildren[nextIndex]) {
+						insertBeforeChildren.push(oldChildren[nextIndex]);
+						nextIndex++;
 					}
 				}
 			}
 
-			createDom(newChild, parentVNode, insertBefore, projectionOptions, parentInstance);
+			createDom(newChild, parentVNode, insertBeforeDomNode, projectionOptions, parentInstance);
 			nodeAdded(newChild, transitions);
 			const indexToCheck = newIndex;
 			projectorState.afterRenderCallbacks.push(() => {
@@ -681,10 +677,15 @@ function updateChildren(
 		const removeChild = () => {
 			const indexToCheck = oldIndex;
 			projectorState.afterRenderCallbacks.push(() => {
-				callOnDetach(oldChild, parentInstance);
 				checkDistinguishable(oldChildren, indexToCheck, parentInstance);
 			});
-			nodeToRemove(oldChild, transitions, projectionOptions);
+			if (isWNode(oldChild)) {
+				const item = instanceMap.get(oldChild.instance);
+				if (item) {
+					oldChild = item.dnode;
+				}
+			}
+			nodeToRemove(oldChild!, transitions, projectionOptions);
 		};
 		const findNewIndex = findIndexOfChild(newChildren, oldChild, newIndex + 1);
 
@@ -702,13 +703,18 @@ function updateChildren(
 	if (oldChildrenLength > oldIndex) {
 		// Remove child fragments
 		for (i = oldIndex; i < oldChildrenLength; i++) {
-			const oldChild = oldChildren[i];
 			const indexToCheck = i;
 			projectorState.afterRenderCallbacks.push(() => {
-				callOnDetach(oldChild, parentInstance);
 				checkDistinguishable(oldChildren, indexToCheck, parentInstance);
 			});
-			nodeToRemove(oldChildren[i], transitions, projectionOptions);
+			let childToRemove = oldChildren[i];
+			if (isWNode(childToRemove)) {
+				const item = instanceMap.get(childToRemove.instance);
+				if (item) {
+					childToRemove = item.dnode;
+				}
+			}
+			nodeToRemove(childToRemove, transitions, projectionOptions);
 		}
 	}
 	return textUpdated;
@@ -895,7 +901,6 @@ function updateDom(
 		instance.__setChildren__(dnode.children);
 		instance.__setProperties__(dnode.properties);
 		dnode.instance = instance;
-		instanceMap.set(instance, { dnode, parentVNode });
 		if (instanceData.dirty === true) {
 			const rendered = instance.__render__();
 			instanceData.rendering = false;
@@ -905,6 +910,7 @@ function updateDom(
 			instanceData.rendering = false;
 			dnode.rendered = previousRendered;
 		}
+		instanceMap.set(instance, { dnode, parentVNode });
 		instanceData.nodeHandler.addRoot();
 	} else {
 		if (previous === dnode) {
@@ -1051,12 +1057,15 @@ function render(projectionOptions: ProjectionOptions) {
 	const renders = [...renderQueue];
 	projectorState.renderQueue = [];
 	renders.sort((a, b) => a.depth - b.depth);
-
+	const previouslyRendered = [];
 	while (renders.length) {
 		const { instance } = renders.shift()!;
-		const { parentVNode, dnode } = instanceMap.get(instance)!;
-		const instanceData = widgetInstanceMap.get(instance)!;
-		updateDom(dnode, toInternalWNode(instance, instanceData), projectionOptions, parentVNode, instance);
+		if (instanceMap.has(instance) && previouslyRendered.indexOf(instance) === -1) {
+			previouslyRendered.push(instance);
+			const { parentVNode, dnode } = instanceMap.get(instance)!;
+			const instanceData = widgetInstanceMap.get(instance)!;
+			updateDom(dnode, toInternalWNode(instance, instanceData), projectionOptions, parentVNode, instance);
+		}
 	}
 	runAfterRenderCallbacks(projectionOptions);
 	runDeferredRenderCallbacks(projectionOptions);
diff --git a/tests/unit/vdom.ts b/tests/unit/vdom.ts
@@ -6,7 +6,7 @@ import sendEvent from './../support/sendEvent';
 
 import { dom, InternalVNode, InternalWNode, widgetInstanceMap, RenderResult } from '../../src/vdom';
 import { dom as d, v, w, VNODE } from '../../src/d';
-import { VNode } from '../../src/interfaces';
+import { VNode, DNode } from '../../src/interfaces';
 import { WidgetBase } from '../../src/WidgetBase';
 import { I18nMixin } from '../../src/mixins/I18n';
 import { Registry } from '../../src/Registry';
@@ -656,6 +656,123 @@ describe('vdom', () => {
 			assert.strictEqual(root.childNodes[4].childNodes[0].data, 'bar');
 		});
 
+		it('should only insert before nodes that are not orphaned', () => {
+			class Parent extends WidgetBase {
+				private items: DNode[] = [w(ChildOne, {}), w(ChildTwo, {})];
+				render() {
+					return v('div', this.items);
+				}
+
+				swap() {
+					this.items = [w(ChildThree, {})];
+					this.invalidate();
+				}
+			}
+
+			let hideOne: Function;
+			class ChildOne extends WidgetBase {
+				private _show = true;
+
+				render() {
+					return this._show ? w(Widget, { num: 1 }) : null;
+				}
+
+				hide() {
+					this._show = false;
+					this.invalidate();
+				}
+
+				constructor() {
+					super();
+					hideOne = this.hide.bind(this);
+				}
+			}
+
+			let hideTwo: Function;
+			class ChildTwo extends WidgetBase {
+				private _show = true;
+
+				render() {
+					return this._show ? w(Widget, { num: 2 }) : null;
+				}
+
+				hide() {
+					this._show = false;
+					this.invalidate();
+				}
+
+				constructor() {
+					super();
+					hideTwo = this.hide.bind(this);
+				}
+			}
+
+			class ChildThree extends WidgetBase {
+				render() {
+					return w(Widget, { num: 3 });
+				}
+			}
+
+			class Widget extends WidgetBase<any> {
+				render() {
+					return v('div', [`hello ${this.properties.num}`]);
+				}
+			}
+
+			const widget = new Parent();
+			const projection = dom.create(widget, { sync: true });
+			hideOne!();
+			hideTwo!();
+			widget.swap();
+			const root: any = projection.domNode.childNodes[0];
+			assert.strictEqual(root.childNodes[0].childNodes[0].data, 'hello 3');
+		});
+
+		it('Should not render widgets that have been detached', () => {
+			class ChildOne extends WidgetBase {
+				render() {
+					return 'Child One';
+				}
+			}
+
+			let childTwoInvalidate: Function;
+			let renderResult: DNode = null;
+			class ChildTwo extends WidgetBase {
+				constructor() {
+					super();
+					childTwoInvalidate = this.invalidate.bind(this);
+				}
+
+				render() {
+					return renderResult;
+				}
+			}
+
+			class Parent extends WidgetBase {
+				private _items: any[] = [w(ChildTwo, {})];
+				render() {
+					return v('main', this._items);
+				}
+
+				switch() {
+					this._items = [w(ChildOne, {})];
+					this.invalidate();
+				}
+			}
+
+			const widget = new Parent();
+			const projection = dom.create(widget);
+			resolvers.resolve();
+			renderResult = v('span', ['me']);
+			childTwoInvalidate!();
+			resolvers.resolve();
+			widget.switch();
+			childTwoInvalidate!();
+			resolvers.resolve();
+			assert.lengthOf(projection.domNode.childNodes[0]!.childNodes, 1);
+			assert.strictEqual((projection.domNode.childNodes[0]!.childNodes[0] as Text).data, 'Child One');
+		});
+
 		it('should allow a widget returned from render', () => {
 			class Bar extends WidgetBase<any> {
 				render() {
