Skip to content

Commit

Permalink
Attempt to fix the router flicker (#337)
Browse files Browse the repository at this point in the history
  • Loading branch information
cristianbote authored Feb 22, 2021
1 parent 6f23e00 commit 438be29
Show file tree
Hide file tree
Showing 11 changed files with 271 additions and 30 deletions.
5 changes: 5 additions & 0 deletions .changeset/wild-brooms-visit.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'preact-iso': patch
---

Show the previous route only for the unresolved thrown routes
6 changes: 3 additions & 3 deletions packages/preact-iso/lazy.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@ import { useState, useRef } from 'preact/hooks';
export default function lazy(load) {
let p, c;
return props => {
if (!p) p = load().then(m => (c = (m && m.default) || m));
const [, update] = useState(0);
const r = useRef(c);
if (!p) p = load().then(m => (c = (m && m.default) || m));
if (c !== undefined) return h(c, props);
if (!r.current) r.current = p.then(() => update(1));
if (c === undefined) throw p;
return h(c, props);
throw p;
};
}

Expand Down
3 changes: 2 additions & 1 deletion packages/preact-iso/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,9 @@
},
"license": "MIT",
"devDependencies": {
"htm": "^3.0.4",
"jest": "26.6.3",
"preact": "^10.5.7",
"preact": "^10.5.12",
"preact-render-to-string": "^5.1.12"
},
"peerDependencies": {
Expand Down
39 changes: 25 additions & 14 deletions packages/preact-iso/router.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { h, createContext, cloneElement } from 'preact';
import { useContext, useMemo, useReducer, useEffect, useRef } from 'preact/hooks';
import { h, createContext, Fragment, cloneElement } from 'preact';
import { useContext, useMemo, useReducer, useEffect, useLayoutEffect, useRef } from 'preact/hooks';

const UPDATE = (state, url, push) => {
if (url && url.type === 'click') {
Expand Down Expand Up @@ -58,7 +58,7 @@ export function LocationProvider(props) {
removeEventListener('click', route);
removeEventListener('popstate', route);
};
});
}, []);

// @ts-ignore
return h(LocationProvider.ctx.Provider, { value }, props.children);
Expand All @@ -81,20 +81,33 @@ export function Router(props) {
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;
}

curChildren.current = useMemo(() => {
let p, d, m;
[].concat(props.children || []).some(vnode => {
const matches = exec(path, vnode.props.path, (m = { path, query }));
if (matches) return (p = cloneElement(vnode, m));
if (vnode.props.default) d = cloneElement(vnode, m);
});

return h(Committer, {}, h(RouteContext.Provider, { value: m }, p || d));
}, [url]);

this.componentDidCatch = err => {
if (err && err.then) pending.current = err;
};

useEffect(() => {
useLayoutEffect(() => {
let p = pending.current;

const commit = () => {
if (cur.current.url !== url || pending.current !== p) return;
prev.current = prevChildren.current = pending.current = null;
if (props.onLoadEnd) props.onLoadEnd(url);
prev.current = prevChildren.current = null;
update(0);
};

Expand All @@ -104,17 +117,15 @@ export function Router(props) {
} else commit();
}, [url]);

let p, d, m;
[].concat(props.children || []).some(vnode => {
const matches = exec(path, vnode.props.path, (m = { path, query }));
if (matches) {
return (p = h(RouteContext.Provider, { value: m }, cloneElement(vnode, m)));
}
// 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 (this.__v && this.__v.__k) this.__v.__k.reverse();

if (vnode.props.default) d = cloneElement(vnode, m);
});
return [curChildren.current, prevChildren.current];
}

return [(curChildren.current = p || d), prevChildren.current];
function Committer({ pending, children }) {
return pending && !pending.current ? null : children;
}

Router.Provider = LocationProvider;
Expand Down
209 changes: 209 additions & 0 deletions packages/preact-iso/test/router.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,209 @@
import { jest, describe, it, beforeEach, expect } from '@jest/globals';
import { h, html, render } from 'htm/preact';
import { LocationProvider, Router, useLocation } from '../router.js';
import lazy, { ErrorBoundary } from '../lazy.js';

const sleep = ms => new Promise(r => setTimeout(r, ms));

// delayed lazy()
const groggy = (component, ms) => lazy(() => sleep(ms).then(() => component));

describe('Router', () => {
let scratch;
beforeEach(() => {
if (scratch) {
render(null, scratch);
scratch.remove();
}
scratch = document.createElement('scratch');
document.body.appendChild(scratch);
history.replaceState(null, null, '/');
});

it('should switch between synchronous routes', async () => {
const Home = jest.fn(() => html`<h1>Home</h1>`);
const Profiles = jest.fn(() => html`<h1>Profiles</h1>`);
const Profile = jest.fn(({ id }) => html`<h1>Profile: ${id}</h1>`);
const Fallback = jest.fn(() => html`<h1>Fallback</h1>`);
let loc;
render(
html`
<${LocationProvider}>
<${Router}>
<${Home} path="/" />
<${Profiles} path="/profiles" />
<${Profile} path="/profiles/:id" />
<${Fallback} default />
<//>
<${() => {
loc = useLocation();
}} />
<//>
`,
scratch
);

expect(scratch).toHaveProperty('textContent', 'Home');
expect(Home).toHaveBeenCalledWith({ path: '/', query: {} }, expect.anything());
expect(Profiles).not.toHaveBeenCalled();
expect(Profile).not.toHaveBeenCalled();
expect(Fallback).not.toHaveBeenCalled();
expect(loc).toMatchObject({
url: '/',
path: '/',
query: {},
route: expect.any(Function)
});

Home.mockReset();
loc.route('/profiles');
await sleep(1);

expect(scratch).toHaveProperty('textContent', 'Profiles');
expect(Home).not.toHaveBeenCalled();
expect(Profiles).toHaveBeenCalledWith({ path: '/profiles', query: {} }, expect.anything());
expect(Profile).not.toHaveBeenCalled();
expect(Fallback).not.toHaveBeenCalled();

expect(loc).toMatchObject({
url: '/profiles',
path: '/profiles',
query: {}
});

Profiles.mockReset();
loc.route('/profiles/bob');
await sleep(1);

expect(scratch).toHaveProperty('textContent', 'Profile: bob');
expect(Home).not.toHaveBeenCalled();
expect(Profiles).not.toHaveBeenCalled();
expect(Profile).toHaveBeenCalledWith({ path: '/profiles/bob', query: {}, id: 'bob' }, expect.anything());
expect(Fallback).not.toHaveBeenCalled();

expect(loc).toMatchObject({
url: '/profiles/bob',
path: '/profiles/bob',
query: {}
});

Profile.mockReset();
loc.route('/other?a=b&c=d');
await sleep(1);

expect(scratch).toHaveProperty('textContent', 'Fallback');
expect(Home).not.toHaveBeenCalled();
expect(Profiles).not.toHaveBeenCalled();
expect(Profile).not.toHaveBeenCalled();
expect(Fallback).toHaveBeenCalledWith(
{ default: true, path: '/other', query: { a: 'b', c: 'd' } },
expect.anything()
);

expect(loc).toMatchObject({
url: '/other?a=b&c=d',
path: '/other',
query: { a: 'b', c: 'd' }
});
});

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));
let loc;
render(
html`
<${ErrorBoundary}>
<${LocationProvider}>
<${Router}>
<${A} path="/" />
<${B} path="/b" />
<${C} path="/c" />
<//>
<${() => {
loc = useLocation();
}} />
<//>
<//>
`,
scratch
);

expect(scratch).toHaveProperty('innerHTML', '');
expect(A).toHaveBeenCalledWith({ path: '/', query: {} }, expect.anything());

A.mockClear();
await sleep(20);

expect(scratch).toHaveProperty('innerHTML', '<h1>A</h1>');
expect(A).toHaveBeenCalledWith({ path: '/', query: {} }, expect.anything());

A.mockClear();
loc.route('/b');

expect(scratch).toHaveProperty('innerHTML', '<h1>A</h1>');
expect(A).not.toHaveBeenCalled();

await sleep(1);

expect(scratch).toHaveProperty('innerHTML', '<h1>A</h1>');
// 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);

expect(scratch).toHaveProperty('innerHTML', '<h1>B</h1>');
expect(A).not.toHaveBeenCalled();
expect(B).toHaveBeenCalledWith({ path: '/b', query: {} }, expect.anything());

B.mockClear();
loc.route('/c');
loc.route('/c?1');
loc.route('/c');

expect(scratch).toHaveProperty('innerHTML', '<h1>B</h1>');
expect(B).not.toHaveBeenCalled();

await sleep(1);

loc.route('/c');
loc.route('/c?2');
loc.route('/c');

expect(scratch).toHaveProperty('innerHTML', '<h1>B</h1>');
// 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);

expect(scratch).toHaveProperty('innerHTML', '<h1>C</h1>');
expect(B).not.toHaveBeenCalled();
expect(C).toHaveBeenCalledWith({ path: '/c', query: {} }, expect.anything());

// "instant" routing to already-loaded routes

C.mockClear();
B.mockClear();
loc.route('/b');
await sleep(1);

expect(scratch).toHaveProperty('innerHTML', '<h1>B</h1>');
expect(C).not.toHaveBeenCalled();
// expect(B).toHaveBeenCalledTimes(1);
expect(B).toHaveBeenCalledWith({ path: '/b', query: {} }, expect.anything());

B.mockClear();
loc.route('/');
await sleep(1);

expect(scratch).toHaveProperty('innerHTML', '<h1>A</h1>');
expect(B).not.toHaveBeenCalled();
// expect(A).toHaveBeenCalledTimes(1);
expect(A).toHaveBeenCalledWith({ path: '/', query: {} }, expect.anything());
});
});
1 change: 1 addition & 0 deletions packages/wmr/demo/public/header.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ export default function Header() {
<nav>
<a href="/">Home</a>
<a href="/about">About</a>
<a href="/lazy-and-late">Lazy and Late</a>
<a href="/compat">Compat</a>
<a href="/class-fields">Class-Fields</a>
<a href="/files">Files</a>
Expand Down
9 changes: 9 additions & 0 deletions packages/wmr/demo/public/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,14 @@ import Header from './header.tsx';
// import './style.css';

const About = lazy(() => import('./pages/about/index.js'));
const LazyAndLate = lazy(
() =>
new Promise(r => {
setTimeout(() => {
r(import('./pages/about/index.js'));
}, 1.5e3);
})
);
const CompatPage = lazy(() => import('./pages/compat.js'));
const ClassFields = lazy(() => import('./pages/class-fields.js'));
const Files = lazy(() => import('./pages/files/index.js'));
Expand All @@ -22,6 +30,7 @@ export function App() {
<Router>
<Home path="/" />
<About path="/about" />
<LazyAndLate path="/lazy-and-late" title={'Lazy and Late'} />
<CompatPage path="/compat" />
<ClassFields path="/class-fields" />
<Files path="/files" />
Expand Down
6 changes: 4 additions & 2 deletions packages/wmr/demo/public/lib/lazy.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,16 @@ import { useState, useRef } from 'preact/hooks';

export default function lazy(load) {
let p, c;
return props => {
function inner(props) {
if (!p) p = load().then(m => ((c = (m && m.default) || m), 1));
const [, update] = useState(0);
const r = useRef(c);
if (!r.current) r.current = p.then(update);
if (c === undefined) throw p;
return h(c, props);
};
}

return inner;
}

export function ErrorBoundary(props) {
Expand Down
Loading

0 comments on commit 438be29

Please sign in to comment.