fix: Ensure same-component routes are rerendered rather than swapped (#16)

* fix: Avoid swapping routes if component hasn't changed

Co-authored-by: Jason Miller <[email protected]>

* fix: Reset routeChanged

* test: Add test impl

---------

Co-authored-by: Jason Miller <[email protected]>

Author: rschristian

src/router.js

@@ -117,24 +117,33 @@ export function Router(props) {
 	// was the most recent render successful (did not suspend):
 	const didSuspend = useRef();
 	didSuspend.current = false;
-
-	useMemo(() => {
-		// This hack prevents Preact from diffing when we swap `cur` to `prev`:
-		if (this.__v && this.__v.__k) this.__v.__k.reverse();
-
-		count.current++;
-		prev.current = cur.current;
-	}, [url]);
+	// has the route component changed
+	const routeChanged = useRef(false);
 
 	let pr, d, m;
 	toChildArray(props.children).some(vnode => {
 		const matches = exec(rest, vnode.props.path, (m = { ...vnode.props, path: rest, query, params, rest: '' }));
 		if (matches) return (pr = cloneElement(vnode, m));
 		if (vnode.props.default) d = cloneElement(vnode, m);
 	});
+
+	let incoming = pr || d;
+	useMemo(() => {
+		prev.current = cur.current;
+
+		// Only mark as an update if the route component changed.
+		const outgoing = prev.current && prev.current.props.children;
+		if (!outgoing || !incoming || incoming.type !== outgoing.type || incoming.props.component !== outgoing.props.component) {
+			// This hack prevents Preact from diffing when we swap `cur` to `prev`:
+			if (this.__v && this.__v.__k) this.__v.__k.reverse();
+			count.current++;
+			routeChanged.current = true;
+		} else routeChanged.current = false;
+	}, [url]);
+
 	const isHydratingSuspense = cur.current && cur.current.__u & MODE_HYDRATE && cur.current.__u & MODE_SUSPENDED;
 	const isHydratingBool = cur.current && cur.current.__h;
-	cur.current =  h(RouteContext.Provider, { value: m }, pr || d);
+	cur.current = h(RouteContext.Provider, { value: m }, incoming);
 	if (isHydratingSuspense) {
 		cur.current.__u |= MODE_HYDRATE;
 		cur.current.__u |= MODE_SUSPENDED;
@@ -220,7 +229,9 @@ export function Router(props) {
 	}, [path, wasPush, c]);
 
 	// Note: curChildren MUST render first in order to set didSuspend & prev.
-	return [h(RenderRef, { r: cur }), h(RenderRef, { r: prev })];
+	return routeChanged.current
+		? [h(RenderRef, { r: cur }), h(RenderRef, { r: prev })]
+		: h(RenderRef, { r: cur });
 }
 
 const MODE_HYDRATE = 1 << 5;

test/router.test.js

@@ -306,6 +306,63 @@ describe('Router', () => {
 		expect(A).toHaveBeenCalledWith({ path: '/', query: {}, params: {}, rest: '' }, expect.anything());
 	});
 
+	it('rerenders same-component routes rather than swap', async () => {
+		const A = jest.fn(groggy(() => html`<h1>a</h1>`, 1));
+		const B = jest.fn(groggy(({ sub }) => html`<h1>b/${sub}</h1>`, 1));
+		let loc, childrenLength;
+
+		const old = options.__c;
+		options.__c = (vnode, queue) => {
+			if (vnode.type === Router) {
+				childrenLength = vnode.__k.length;
+			}
+
+			if (old) old(vnode, queue);
+		}
+
+		render(
+			html`
+				<${ErrorBoundary}>
+					<${LocationProvider}>
+						<${Router}>
+							<${A} path="/" />
+							<${B} path="/b/:sub" />
+						<//>
+						<${() => {
+							loc = useLocation();
+						}} />
+					<//>
+				<//>
+			`,
+			scratch
+		);
+
+		await sleep(10);
+
+		expect(scratch).toHaveProperty('innerHTML', '<h1>a</h1>');
+		expect(childrenLength).toBe(2);
+
+		loc.route('/b/a');
+		await sleep(10);
+
+		expect(scratch).toHaveProperty('innerHTML', '<h1>b/a</h1>');
+		expect(childrenLength).toBe(2);
+
+		loc.route('/b/b');
+		await sleep(10);
+
+		expect(scratch).toHaveProperty('innerHTML', '<h1>b/b</h1>');
+		expect(childrenLength).toBe(1);
+
+		loc.route('/');
+		await sleep(10);
+
+		expect(scratch).toHaveProperty('innerHTML', '<h1>a</h1>');
+		expect(childrenLength).toBe(2);
+
+		options.__c = old;
+	});
+
 	it('should support onLoadStart/onLoadEnd/onRouteChange w/out navigation', async () => {
 		const route = name => html`
 			<h1>${name}</h1>