Merge pull request #2910 from tanhauhau/tanhauhau/fix-suspense-updati…

…ng-suspended-component

compat/src/suspense.js

@@ -22,7 +22,7 @@ options._catchError = function(error, newVNode, oldVNode) {
 	oldCatchError(error, newVNode, oldVNode);
 };
 
-function detachedClone(vnode) {
+function detachedClone(vnode, detachedParent, parentDom) {
 	if (vnode) {
 		if (vnode._component && vnode._component.__hooks) {
 			vnode._component.__hooks._list.forEach(effect => {
@@ -33,17 +33,39 @@ function detachedClone(vnode) {
 		}
 
 		vnode = assign({}, vnode);
-		vnode._component = null;
-		vnode._children = vnode._children && vnode._children.map(detachedClone);
+		if (vnode._component != null) {
+			if (vnode._component._parentDom === parentDom) {
+				vnode._component._parentDom = detachedParent;
+			}
+			vnode._component = null;
+		}
+		vnode._children =
+			vnode._children &&
+			vnode._children.map(child =>
+				detachedClone(child, detachedParent, parentDom)
+			);
 	}
 
 	return vnode;
 }
 
-function removeOriginal(vnode) {
+function removeOriginal(vnode, detachedParent, originalParent) {
 	if (vnode) {
 		vnode._original = null;
-		vnode._children = vnode._children && vnode._children.map(removeOriginal);
+		vnode._children =
+			vnode._children &&
+			vnode._children.map(child =>
+				removeOriginal(child, detachedParent, originalParent)
+			);
+		if (vnode._component) {
+			if (vnode._component._parentDom === detachedParent) {
+				if (vnode._dom) {
+					originalParent.insertBefore(vnode._dom, vnode._nextDom);
+				}
+				vnode._component._force = true;
+				vnode._component._parentDom = originalParent;
+			}
+		}
 	}
 	return vnode;
 }
@@ -103,13 +125,32 @@ Suspense.prototype._childDidSuspend = function(promise, suspendingVNode) {
 			suspendingComponent._suspendedComponentWillUnmount();
 		}
 	};
+	const _suspendedRender = suspendingComponent.render;
+	suspendingComponent.render = (props, state, context) => {
+		const result = _suspendedRender.call(
+			suspendingComponent,
+			props,
+			state,
+			context
+		);
+
+		suspendingComponent._renderCallbacks.push(onResolved);
+
+		suspendingComponent.render = _suspendedRender;
+		return result;
+	};
 
 	const onSuspensionComplete = () => {
 		if (!--c._pendingSuspensionCount) {
 			// If the suspension was during hydration we don't need to restore the
 			// suspended children into the _children array
 			if (c.state._suspended) {
-				c._vnode._children[0] = removeOriginal(c.state._suspended);
+				const suspendedVNode = c.state._suspended;
+				c._vnode._children[0] = removeOriginal(
+					suspendedVNode,
+					suspendedVNode._component._parentDom,
+					suspendedVNode._component._originalParentDom
+				);
 			}
 
 			c.setState({ _suspended: (c._detachOnNextRender = null) });
@@ -148,7 +189,13 @@ Suspense.prototype.render = function(props, state) {
 		// (i.e. due to a setState further up in the tree)
 		// it's _children prop is null, in this case we "forget" about the parked vnodes to detach
 		if (this._vnode._children) {
-			this._vnode._children[0] = detachedClone(this._detachOnNextRender);
+			const detachedParent = document.createElement('div');
+			const detachedComponent = this._vnode._children[0]._component;
+			this._vnode._children[0] = detachedClone(
+				this._detachOnNextRender,
+				detachedParent,
+				(detachedComponent._originalParentDom = detachedComponent._parentDom)
+			);
 		}
 
 		this._detachOnNextRender = null;

compat/test/browser/suspense.test.js

@@ -659,8 +659,7 @@ describe('suspense', () => {
 		});
 	});
 
-	// TODO: Fix this test
-	it.skip('should allow children to update state while suspending', () => {
+	it('should allow children to update state while suspending', () => {
 		/** @type {(state: { s: string }) => void} */
 		let setState;
 		class Stateful extends Component {
@@ -1560,6 +1559,62 @@ describe('suspense', () => {
 		expect(scratch.innerHTML).to.eql('<div>conditional hide</div>');
 	});
 
+	it('should support updating state while suspended', async () => {
+		const [Suspender, suspend] = createSuspender(() => <div>Suspender</div>);
+
+		let increment;
+
+		class Updater extends Component {
+			constructor(props) {
+				super(props);
+				this.state = { i: 0 };
+
+				increment = () => {
+					this.setState(({ i }) => ({ i: i + 1 }));
+				};
+			}
+
+			render(props, { i }) {
+				return (
+					<div>
+						i: {i}
+						<Suspender />
+					</div>
+				);
+			}
+		}
+
+		render(
+			<Suspense fallback={<div>Suspended...</div>}>
+				<Updater />
+			</Suspense>,
+			scratch
+		);
+
+		expect(scratch.innerHTML).to.eql(`<div>i: 0<div>Suspender</div></div>`);
+		expect(Suspender.prototype.render).to.have.been.calledOnce;
+
+		const [resolve] = suspend();
+		rerender();
+
+		expect(scratch.innerHTML).to.eql(`<div>Suspended...</div>`);
+
+		increment();
+		rerender();
+
+		expect(scratch.innerHTML).to.eql(`<div>Suspended...</div>`);
+
+		await resolve(() => <div>Resolved</div>);
+		rerender();
+
+		expect(scratch.innerHTML).to.equal(`<div>i: 1<div>Resolved</div></div>`);
+
+		increment();
+		rerender();
+
+		expect(scratch.innerHTML).to.equal(`<div>i: 2<div>Resolved</div></div>`);
+	});
+
 	it('should call componentWillUnmount on a suspended component', () => {
 		const cWUSpy = sinon.spy();
 
@@ -1622,7 +1677,7 @@ describe('suspense', () => {
 		expect(scratch.innerHTML).to.eql(`<div>conditional hide</div>`);
 	});
 
-	xit('should support sCU=false when un-suspending', () => {
+	it('should support sCU=false when un-suspending', () => {
 		// See #2176 #2125
 		const [Suspender, suspend] = createSuspender(() => <div>Hello</div>);
 
@@ -1650,7 +1705,7 @@ describe('suspense', () => {
 		});
 	});
 
-	xit('should allow suspended children to update', () => {
+	it('should allow suspended children to update', () => {
 		const log = [];
 		class Logger extends Component {
 			constructor(props) {
@@ -1681,7 +1736,7 @@ describe('suspense', () => {
 					throw this.state.promise;
 				}
 
-				return 'hello';
+				return <div>Suspender un-suspended</div>;
 			}
 		}
 
@@ -1706,25 +1761,141 @@ describe('suspense', () => {
 
 		rerender();
 
-		/**
-		 * These currently failing assertion shows the issue that we currently unmount
-		 * the suspended tree (unlike react, which adds a display="none") and block any
-		 * further processing on that tree. Thus updates below a suspended Suspense are
-		 * getting lost.
-		 */
 		expect(log).to.eql(['construct', 'render', 'render']);
-
-		/**
-		 * When the above assertion will hold true we will certainly run into the second issue
-		 * here. The problem is that we do not remove suspensions from an instance of Suspense
-		 * when one of its suspending children no longer throws because of a state
-		 * update.
-		 */
 		expect(scratch.innerHTML).to.eql(
 			'<section><div>Suspender un-suspended</div></section>'
 		);
 	});
 
+	it('should allow multiple suspended children to update', () => {
+		function createSuspender() {
+			let suspender;
+			class Suspender extends Component {
+				constructor(props) {
+					super(props);
+					this.state = { promise: new Promise(() => {}) };
+					suspender = this;
+				}
+
+				unsuspend(content) {
+					this.setState({ promise: null, content });
+				}
+
+				render() {
+					if (this.state.promise) {
+						throw this.state.promise;
+					}
+
+					return this.state.content;
+				}
+			}
+			return [content => suspender.unsuspend(content), Suspender];
+		}
+
+		const [unsuspender1, Suspender1] = createSuspender();
+		const [unsuspender2, Suspender2] = createSuspender();
+
+		render(
+			<section>
+				<Suspense fallback={<div>fallback</div>}>
+					<Suspender1 />
+					<div>
+						<Suspender2 />
+					</div>
+				</Suspense>
+			</section>,
+			scratch
+		);
+
+		expect(scratch.innerHTML).to.eql('<section><div></div></section>');
+
+		// this rerender is needed because of Suspense issuing a forceUpdate itself
+		rerender();
+		expect(scratch.innerHTML).to.eql('<section><div>fallback</div></section>');
+
+		unsuspender1(
+			<>
+				<div>Suspender un-suspended 1</div>
+				<div>Suspender un-suspended 2</div>
+			</>
+		);
+
+		rerender();
+		expect(scratch.innerHTML).to.eql('<section><div>fallback</div></section>');
+
+		unsuspender2(<div>Suspender 2</div>);
+
+		rerender();
+		expect(scratch.innerHTML).to.eql(
+			'<section><div>Suspender un-suspended 1</div><div>Suspender un-suspended 2</div><div><div>Suspender 2</div></div></section>'
+		);
+	});
+
+	it('should allow suspended children children to update', () => {
+		function Suspender({ promise, content }) {
+			if (promise) {
+				throw promise;
+			}
+			return content;
+		}
+
+		let parent;
+		class Parent extends Component {
+			constructor(props) {
+				super(props);
+				this.state = { promise: new Promise(() => {}), condition: true };
+				parent = this;
+			}
+
+			render() {
+				const { condition, promise, content } = this.state;
+				if (condition) {
+					return <Suspender promise={promise} content={content} />;
+				}
+				return <div>Parent</div>;
+			}
+		}
+
+		render(
+			<section>
+				<Suspense fallback={<div>fallback</div>}>
+					<Parent />
+				</Suspense>
+			</section>,
+			scratch
+		);
+
+		expect(scratch.innerHTML).to.eql('<section></section>');
+
+		// this rerender is needed because of Suspense issuing a forceUpdate itself
+		rerender();
+		expect(scratch.innerHTML).to.eql('<section><div>fallback</div></section>');
+
+		// hide the <Suspender /> thus unsuspends
+		parent.setState({ condition: false });
+
+		rerender();
+		expect(scratch.innerHTML).to.eql('<section><div>Parent</div></section>');
+
+		// show the <Suspender /> thus re-suspends
+		parent.setState({ condition: true });
+		rerender();
+
+		expect(scratch.innerHTML).to.eql('<section><div>fallback</div></section>');
+
+		// update state so that <Suspender /> no longer suspends
+		parent.setState({ promise: null, content: <div>Content</div> });
+		rerender();
+
+		expect(scratch.innerHTML).to.eql('<section><div>Content</div></section>');
+
+		// hide the <Suspender /> again
+		parent.setState({ condition: false });
+		rerender();
+
+		expect(scratch.innerHTML).to.eql('<section><div>Parent</div></section>');
+	});
+
 	it('should render delayed lazy components through components using shouldComponentUpdate', () => {
 		const [Suspender1, suspend1] = createSuspender(() => <i>1</i>);
 		const [Suspender2, suspend2] = createSuspender(() => <i>2</i>);

mangle.json

@@ -61,6 +61,7 @@
       "$_id": "__c",
       "$_contextRef": "__",
       "$_parentDom": "__P",
+      "$_originalParentDom": "__O",
       "$_prevState": "__u",
       "$_root": "__",
       "$_diff": "__b",
@@ -72,7 +73,8 @@
       "$_owner": "__o",
       "$_skipEffects": "__s",
       "$_rerenderCount": "__r",
-      "$_forwarded": "__f"
+      "$_forwarded": "__f",
+      "$_isSuspended": "__i"
     }
   }
 }