Merge pull request #504 from preactjs/bugfix-iso-route-flashing-3

.changeset/odd-singers-explode.md

@@ -0,0 +1,5 @@
+---
+"preact-iso": patch
+---
+
+Bugfix: fix route flashing for routes that render fragments

packages/preact-iso/router.js

@@ -78,15 +78,18 @@ export function Router(props) {
 	const prevChildren = useRef();
 	const pending = useRef();
 
-	let reverse = false;
 	if (url !== cur.current.url) {
-		reverse = true;
 		pending.current = null;
 		prev.current = cur.current;
 		prevChildren.current = curChildren.current;
 		// old <Committer> uses the pending promise ref to know whether to render
 		prevChildren.current.props.pending = pending;
 		cur.current = loc;
+
+		// Hi! Wondering what this horrid line is for? That's totally reasonable, it is gross.
+		// It prevents the old route from being remounted because it got shifted in the children Array.
+		// @ts-ignore-next
+		if (this.__v && this.__v.__k) this.__v.__k.reverse();
 	}
 
 	curChildren.current = useMemo(() => {
@@ -120,11 +123,7 @@ export function Router(props) {
 		} else commit();
 	}, [url]);
 
-	// Hi! Wondering what this horrid line is for? That's totally reasonable, it is gross.
-	// It prevents the old route from being remounted because it got shifted in the children Array.
-	if (reverse && this.__v && this.__v.__k) this.__v.__k.reverse();
-
-	return [curChildren.current, prevChildren.current];
+	return [prevChildren.current, curChildren.current];
 }
 
 function Committer({ pending, children }) {

packages/preact-iso/test/router.test.js

@@ -109,9 +109,13 @@ describe('Router', () => {
 	});
 
 	it('should wait for asynchronous routes', async () => {
-		const A = jest.fn(groggy(() => html`<h1>A</h1>`, 10));
-		const B = jest.fn(groggy(() => html`<h1>B</h1>`, 10));
-		const C = jest.fn(groggy(() => html`<h1>C</h1>`, 10));
+		const route = name => html`
+			<h1>${name}</h1>
+			<p>hello</p>
+		`;
+		const A = jest.fn(groggy(() => route('A'), 1));
+		const B = jest.fn(groggy(() => route('B'), 1));
+		const C = jest.fn(groggy(() => html`<h1>C</h1>`, 1));
 		let loc;
 		render(
 			html`
@@ -135,28 +139,28 @@ describe('Router', () => {
 		expect(A).toHaveBeenCalledWith({ path: '/', query: {} }, expect.anything());
 
 		A.mockClear();
-		await sleep(20);
+		await sleep(10);
 
-		expect(scratch).toHaveProperty('innerHTML', '<h1>A</h1>');
+		expect(scratch).toHaveProperty('innerHTML', '<h1>A</h1><p>hello</p>');
 		expect(A).toHaveBeenCalledWith({ path: '/', query: {} }, expect.anything());
 
 		A.mockClear();
 		loc.route('/b');
 
-		expect(scratch).toHaveProperty('innerHTML', '<h1>A</h1>');
+		expect(scratch).toHaveProperty('innerHTML', '<h1>A</h1><p>hello</p>');
 		expect(A).not.toHaveBeenCalled();
 
 		await sleep(1);
 
-		expect(scratch).toHaveProperty('innerHTML', '<h1>A</h1>');
+		expect(scratch).toHaveProperty('innerHTML', '<h1>A</h1><p>hello</p>');
 		// We should never re-invoke <A /> while loading <B /> (that would be a remount of the old route):
 		expect(A).not.toHaveBeenCalled();
 		expect(B).toHaveBeenCalledWith({ path: '/b', query: {} }, expect.anything());
 
 		B.mockClear();
-		await sleep(20);
+		await sleep(10);
 
-		expect(scratch).toHaveProperty('innerHTML', '<h1>B</h1>');
+		expect(scratch).toHaveProperty('innerHTML', '<h1>B</h1><p>hello</p>');
 		expect(A).not.toHaveBeenCalled();
 		expect(B).toHaveBeenCalledWith({ path: '/b', query: {} }, expect.anything());
 
@@ -165,7 +169,7 @@ describe('Router', () => {
 		loc.route('/c?1');
 		loc.route('/c');
 
-		expect(scratch).toHaveProperty('innerHTML', '<h1>B</h1>');
+		expect(scratch).toHaveProperty('innerHTML', '<h1>B</h1><p>hello</p>');
 		expect(B).not.toHaveBeenCalled();
 
 		await sleep(1);
@@ -174,13 +178,13 @@ describe('Router', () => {
 		loc.route('/c?2');
 		loc.route('/c');
 
-		expect(scratch).toHaveProperty('innerHTML', '<h1>B</h1>');
+		expect(scratch).toHaveProperty('innerHTML', '<h1>B</h1><p>hello</p>');
 		// We should never re-invoke <A /> while loading <B /> (that would be a remount of the old route):
 		expect(B).not.toHaveBeenCalled();
 		expect(C).toHaveBeenCalledWith({ path: '/c', query: {} }, expect.anything());
 
 		C.mockClear();
-		await sleep(20);
+		await sleep(10);
 
 		expect(scratch).toHaveProperty('innerHTML', '<h1>C</h1>');
 		expect(B).not.toHaveBeenCalled();
@@ -193,7 +197,7 @@ describe('Router', () => {
 		loc.route('/b');
 		await sleep(1);
 
-		expect(scratch).toHaveProperty('innerHTML', '<h1>B</h1>');
+		expect(scratch).toHaveProperty('innerHTML', '<h1>B</h1><p>hello</p>');
 		expect(C).not.toHaveBeenCalled();
 		// expect(B).toHaveBeenCalledTimes(1);
 		expect(B).toHaveBeenCalledWith({ path: '/b', query: {} }, expect.anything());
@@ -202,7 +206,7 @@ describe('Router', () => {
 		loc.route('/');
 		await sleep(1);
 
-		expect(scratch).toHaveProperty('innerHTML', '<h1>A</h1>');
+		expect(scratch).toHaveProperty('innerHTML', '<h1>A</h1><p>hello</p>');
 		expect(B).not.toHaveBeenCalled();
 		// expect(A).toHaveBeenCalledTimes(1);
 		expect(A).toHaveBeenCalledWith({ path: '/', query: {} }, expect.anything());