fix: Ensure same-component routes are rerendered rather than swapped #16
Conversation
rschristian
changed the title
Co-authored-by: Jason Miller <developit@users.noreply.github.com>
rschristian
force-pushed
the
fix/unrendering-same-component-route
branch
2 times, most recently
from
711a560
to
6607652
Compare
rschristian
force-pushed
the
fix/unrendering-same-component-route
branch
from
6607652
to
2e4d850
Compare
|
|
||
| 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; |
There was a problem hiding this comment.
We should really go for better naming in this library and build it instead 😅 the pr and d stuff never fails to confuse me
There was a problem hiding this comment.
Every time I need to delve in here I have to relearn what in the world our variables are meant to stand for
Totally agree
|
There's a general point to be made of Preact optimising these route-changes because there is no point in diffing, just unmount the root node and do an optimised mont route |
|
I still need to figure out a proper test case / test cases, played around with it today but didn't find a non-awful solution.
IIUC, you're saying it's best to drop umount & recreate same-component routes? While I agree in the specific case of preact-www, and similar-ish sites/pages, I'm not sure if I'd agree in general. While it might perform better, we're potentially blowing away a lot of UI state doing that, e.g., on the Preact guide, clicking any link in the sidebar will completely reset the sidebar scroll position. While there's certainly some options to fix that in other ways, I'm not sure most users expect a router to completely blow away the page content on same nav. |
|
I mainly meant everything belonging to that route should be unmounted immediately, not the full page 😅 currently we still go into a |
This is how things work as-is, no? Or you can set it up that way w/ components outside of the router, anyhow. For routes w/ different components, dropping & recreating the routes is probably a safe no-brainer, it's just a question of whether or not it's generally worth it for routes w/ same components. With this PR I'm guessing it's better, though I suppose it depends on ecosystem and might not be generalizable. I'm not sure if there's any heuristic beyond pushing for subrouters, I'd think that's the only solution? Apologies if I'm misunderstanding you 😅 |
|
I am talking about Preact itself needs a solution in core for this |
|
Ah, gotcha, sorry |
Avoids swapping out component if the route hasn't changed. I remember seeing Jason ran into this on Mastodawn, modified his implementation a bit for here.
I'm not 100% sure of this implementation, but the issue I'm looking to fix is that same-component navigations end up dropping the previous for the new which can be pretty wasteful & slow. I believe diffing should win out for same-component routes more often than not, which is the main change here.
This is especially bad on the Preact docs site, where navigating via the sidebar replaces the entire page (minus the header) as we have a rather "fat" routing setup.