Make suspense work like it does in React

preactjs · May 4, 2019 · 5036536 · 5036536
1 parent b8bb86e
commit 5036536
Show file tree

Hide file tree

Showing 4 changed files with 222 additions and 147 deletions.
diff --git a/src/diff/index.js b/src/diff/index.js
@@ -5,7 +5,6 @@ import { diffChildren } from './children';
 import { diffProps } from './props';
 import { assign, removeNode } from '../util';
 import options from '../options';
-import { sym as suspenseSymbol } from '../suspense';
 
 /**
  * Diff two virtual nodes and apply proper changes to the DOM
@@ -27,19 +26,7 @@ import { sym as suspenseSymbol } from '../suspense';
 export function diff(parentDom, newVNode, oldVNode, context, isSvg, excessDomChildren, mounts, ancestorComponent, force, oldDom) {
 	// If the previous type doesn't match the new type we drop the whole subtree
 	if (oldVNode==null || newVNode==null || oldVNode.type!==newVNode.type || oldVNode.key!==newVNode.key) {
-		if (oldVNode!=null) {
-			const ancestor = oldVNode._component && oldVNode._component._ancestorComponent;
-			if (ancestor && ancestor[suspenseSymbol]) {
-				ancestor._vnode.suspendedContent = oldVNode;
-				// TODO: check whether parent was already removed from DOM...
-				if (oldVNode._dom) {
-					removeNode(oldVNode._dom);
-				}
-			}
-			else {
-				unmount(oldVNode, ancestorComponent);
-			}
-		}
+		if (oldVNode!=null) unmount(oldVNode, ancestorComponent);
 		if (newVNode==null) return null;
 		oldVNode = EMPTY_OBJ;
 	}
@@ -56,6 +43,9 @@ export function diff(parentDom, newVNode, oldVNode, context, isSvg, excessDomChi
 
 	try {
 		outer: if (oldVNode.type===Fragment || newType===Fragment) {
+			// Passing the ancestorComponent here is needed for catchErrorInComponent to properly traverse upwards through fragments
+			// to find a parent Suspense
+			c = { _ancestorComponent: ancestorComponent };
 			diffChildren(parentDom, newVNode, oldVNode, context, isSvg, excessDomChildren, mounts, c, oldDom);
 
 			// Mark dom as empty in case `_children` is any empty array. If it isn't
@@ -153,11 +143,6 @@ export function diff(parentDom, newVNode, oldVNode, context, isSvg, excessDomChi
 
 			let prev = c._prevVNode || null;
 			c._dirty = false;
-
-			if (c[suspenseSymbol] === suspenseSymbol && c._vnode.suspendedContent) {
-				if (prev) { unmount(prev); }
-				prev = c._vnode.suspendedContent;
-			}
 			let vnode = c._prevVNode = coerceToVNode(c.render(c.props, c.state, c.context));
 
 			if (c.getChildContext!=null) {
@@ -169,7 +154,6 @@ export function diff(parentDom, newVNode, oldVNode, context, isSvg, excessDomChi
 			}
 
 			c._depth = ancestorComponent ? (ancestorComponent._depth || 0) + 1 : 0;
-			// this creates a new WrapperOne / new CustomSuspense
 			c.base = newVNode._dom = diff(parentDom, vnode, prev, context, isSvg, excessDomChildren, mounts, c, null, oldDom);
 
 			if (vnode!=null) {
@@ -389,10 +373,8 @@ function catchErrorInComponent(error, component) {
 		if (!component._processingException) {
 			try {
 				if (isSuspend) {
-					// console.log('catchErrorInComponent component[suspenseSymbol]', component[suspenseSymbol]);
-					if (component[suspenseSymbol] === suspenseSymbol && component.componentDidCatch!=null) {
-						// console.log('hitting suspense...');
-						component.componentDidCatch(error);
+					if (component.__s) {
+						component.__s(error);
 					}
 					else {
 						continue;

diff --git a/src/suspense.js b/src/suspense.js
@@ -1,30 +1,17 @@
 import { Component } from './component';
 import { createElement } from './create-element';
 
-// TODO: react warns in dev mode about defaultProps and propTypes not being supported on lazy
-// loaded components
-
-export const sym = '_s';
-
-export const Suspense2 = 'suspense';
-
 export class Suspense extends Component {
 	constructor(props) {
-		// TODO: should we add propTypes in DEV mode?
 		super(props);
 
-		// mark this component as a handler of suspension (thrown Promises)
-		this[sym] = sym;
-
-		this.state = {
-			l: false
-		};
+		this.state = {};
 	}
 
-	componentDidCatch(e) {
-		if (e && typeof e.then === 'function') {
-			this.setState({ l: true });
-			const cb = () => { this.setState({ l: false }); };
+	__s(e) {
+		if (typeof e.then == 'function') {
+			this.setState({ l: 1 });
+			const cb = () => { this.setState({ l: 0 }); };
 
 			// Suspense ignores errors thrown in Promises as this should be handled by user land code
 			e.then(cb, cb);
@@ -34,16 +21,16 @@ export class Suspense extends Component {
 		}
 	}
 
-	render() {
-		return this.state.l ? this.props.fallback : this.props.children;
+	render(props, state) {
+		return state.l ? props.fallback : props.children;
 	}
 }
 
 export function lazy(loader) {
 	let prom;
 	let component;
 	let error;
-	return function Lazy(props) {
+	return function L(props) {
 		if (!prom) {
 			prom = loader();
 			prom.then(
@@ -62,4 +49,4 @@ export function lazy(loader) {
 
 		return createElement(component, props);
 	};
-}
+}
diff --git a/test/browser/lifecycle.test.js b/test/browser/lifecycle.test.js
@@ -1,5 +1,5 @@
 import { setupRerender } from 'preact/test-utils';
-import { createElement as h, render, Component } from '../../src/index';
+import { createElement as h, render, Component, Fragment } from '../../src/index';
 import { setupScratch, teardown } from '../_util/helpers';
 
 /** @jsx h */
@@ -2245,6 +2245,15 @@ describe('Lifecycle methods', () => {
 			expect(Receiver.prototype.componentDidCatch).to.have.been.called;
 		});
 
+		it('should be called when child inside a Fragment fails', () => {
+			function ThrowErr() {
+				throw new Error('Error!');
+			}
+
+			render(<Receiver><Fragment><ThrowErr /></Fragment></Receiver>, scratch);
+			expect(Receiver.prototype.componentDidCatch).to.have.been.called;
+		});
+
 		it('should re-render with new content', () => {
 			class ThrowErr extends Component {
 				componentWillMount() {