use options.unmount instead of overriding component.componentWillUnmount (#2919)

* use options.unmount instead of overriding component.componentWillUnmount

* add test cases, allow hydrated suspended component can be unmounted

Author: tanhauhau

compat/src/internal.d.ts

@@ -17,7 +17,7 @@ export interface Component<P = {}, S = {}> extends PreactComponent<P, S> {
 	// Suspense internal properties
 	_childDidSuspend?(error: Promise<void>, suspendingVNode: VNode): void;
 	_suspended: (vnode: VNode) => (unsuspend: () => void) => void;
-	_suspendedComponentWillUnmount?(): void;
+	_onResolve?(): void;
 
 	// Portal internal properties
 	_temp: any;

compat/src/suspense.js

@@ -22,6 +22,25 @@ options._catchError = function(error, newVNode, oldVNode) {
 	oldCatchError(error, newVNode, oldVNode);
 };
 
+const oldUnmount = options.unmount;
+options.unmount = function(vnode) {
+	/** @type {import('./internal').Component} */
+	const component = vnode._component;
+	if (component && component._onResolve) {
+		component._onResolve();
+	}
+
+	// if the component is still hydrating
+	// most likely it is because the component is suspended
+	// we set the vnode.type as `null` so that it is not a typeof function
+	// so the unmount will remove the vnode._dom
+	if (component && vnode._hydrating === true) {
+		vnode.type = null;
+	}
+
+	if (oldUnmount) oldUnmount(vnode);
+};
+
 function detachedClone(vnode, detachedParent, parentDom) {
 	if (vnode) {
 		if (vnode._component && vnode._component.__hooks) {
@@ -109,8 +128,7 @@ Suspense.prototype._childDidSuspend = function(promise, suspendingVNode) {
 		if (resolved) return;
 
 		resolved = true;
-		suspendingComponent.componentWillUnmount =
-			suspendingComponent._suspendedComponentWillUnmount;
+		suspendingComponent._onResolve = null;
 
 		if (resolve) {
 			resolve(onSuspensionComplete);
@@ -119,15 +137,7 @@ Suspense.prototype._childDidSuspend = function(promise, suspendingVNode) {
 		}
 	};
 
-	suspendingComponent._suspendedComponentWillUnmount =
-		suspendingComponent.componentWillUnmount;
-	suspendingComponent.componentWillUnmount = () => {
-		onResolved();
-
-		if (suspendingComponent._suspendedComponentWillUnmount) {
-			suspendingComponent._suspendedComponentWillUnmount();
-		}
-	};
+	suspendingComponent._onResolve = onResolved;
 
 	const onSuspensionComplete = () => {
 		if (!--c._pendingSuspensionCount) {

compat/src/util.js

@@ -21,3 +21,8 @@ export function shallowDiffers(a, b) {
 	for (let i in b) if (i !== '__source' && a[i] !== b[i]) return true;
 	return false;
 }
+
+export function removeNode(node) {
+	let parentNode = node.parentNode;
+	if (parentNode) parentNode.removeChild(node);
+}

compat/test/browser/suspense-hydration.test.js

@@ -182,6 +182,140 @@ describe('suspense hydration', () => {
 		});
 	});
 
+	it('should allow parents to update around suspense boundary and unmount', async () => {
+		scratch.innerHTML = '<div>Count: 0</div><div>Hello</div>';
+		clearLog();
+
+		const [Lazy, resolve] = createLazy();
+
+		/** @type {() => void} */
+		let increment;
+		function Counter() {
+			const [count, setCount] = useState(0);
+			increment = () => setCount(c => c + 1);
+			return (
+				<Fragment>
+					<div>Count: {count}</div>
+					<Suspense>
+						<Lazy />
+					</Suspense>
+				</Fragment>
+			);
+		}
+
+		let hide;
+		function Component() {
+			const [show, setShow] = useState(true);
+			hide = () => setShow(false);
+
+			return show ? <Counter /> : null;
+		}
+
+		hydrate(<Component />, scratch);
+		rerender(); // Flush rerender queue to mimic what preact will really do
+		expect(scratch.innerHTML).to.equal('<div>Count: 0</div><div>Hello</div>');
+		// Re: DOM OP below - Known issue with hydrating merged text nodes
+		expect(getLog()).to.deep.equal(['<div>Count: .appendChild(#text)']);
+		clearLog();
+
+		increment();
+		rerender();
+
+		expect(scratch.innerHTML).to.equal('<div>Count: 1</div><div>Hello</div>');
+		expect(getLog()).to.deep.equal([]);
+		clearLog();
+
+		await resolve(() => <div>Hello</div>);
+		rerender();
+		expect(scratch.innerHTML).to.equal('<div>Count: 1</div><div>Hello</div>');
+		expect(getLog()).to.deep.equal([]);
+		clearLog();
+
+		hide();
+		rerender();
+		expect(scratch.innerHTML).to.equal('');
+	});
+
+	it('should allow parents to update around suspense boundary and unmount before resolves', async () => {
+		scratch.innerHTML = '<div>Count: 0</div><div>Hello</div>';
+		clearLog();
+
+		const [Lazy] = createLazy();
+
+		/** @type {() => void} */
+		let increment;
+		function Counter() {
+			const [count, setCount] = useState(0);
+			increment = () => setCount(c => c + 1);
+			return (
+				<Fragment>
+					<div>Count: {count}</div>
+					<Suspense>
+						<Lazy />
+					</Suspense>
+				</Fragment>
+			);
+		}
+
+		let hide;
+		function Component() {
+			const [show, setShow] = useState(true);
+			hide = () => setShow(false);
+
+			return show ? <Counter /> : null;
+		}
+
+		hydrate(<Component />, scratch);
+		rerender(); // Flush rerender queue to mimic what preact will really do
+		expect(scratch.innerHTML).to.equal('<div>Count: 0</div><div>Hello</div>');
+		// Re: DOM OP below - Known issue with hydrating merged text nodes
+		expect(getLog()).to.deep.equal(['<div>Count: .appendChild(#text)']);
+		clearLog();
+
+		increment();
+		rerender();
+
+		expect(scratch.innerHTML).to.equal('<div>Count: 1</div><div>Hello</div>');
+		expect(getLog()).to.deep.equal([]);
+		clearLog();
+
+		hide();
+		rerender();
+		expect(scratch.innerHTML).to.equal('');
+	});
+
+	it('should allow parents to unmount before resolves', async () => {
+		scratch.innerHTML = '<div>Count: 0</div><div>Hello</div>';
+
+		const [Lazy] = createLazy();
+
+		function Counter() {
+			return (
+				<Fragment>
+					<div>Count: 0</div>
+					<Suspense>
+						<Lazy />
+					</Suspense>
+				</Fragment>
+			);
+		}
+
+		let hide;
+		function Component() {
+			const [show, setShow] = useState(true);
+			hide = () => setShow(false);
+
+			return show ? <Counter /> : null;
+		}
+
+		hydrate(<Component />, scratch);
+		rerender(); // Flush rerender queue to mimic what preact will really do
+
+		hide();
+		rerender();
+		expect(scratch.innerHTML).to.equal('');
+	});
+
 	it('should properly hydrate when there is DOM and Components between Suspense and suspender', () => {
 		scratch.innerHTML = '<div><div>Hello</div></div>';
 		clearLog();

mangle.json

@@ -46,7 +46,7 @@
       "$_children": "__k",
       "$_pendingSuspensionCount": "__u",
       "$_childDidSuspend": "__c",
-      "$_suspendedComponentWillUnmount": "__c",
+      "$_onResolve": "__R",
       "$_suspended": "__e",
       "$_dom": "__e",
       "$_hydrating": "__h",