-
-
Notifications
You must be signed in to change notification settings - Fork 108
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(preact-iso) - handle cancelled renders #527
Conversation
|
Size Change: +170 B (0%) Total Size: 734 kB
ℹ️ View Unchanged
|
@@ -3,23 +3,23 @@ import { useState, useRef } from 'preact/hooks'; | |||
|
|||
export default function lazy(load) { | |||
let p, c; | |||
function inner(props) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't mind the demo/public/lazy/*.js
they weren't updated yet, best to look at the router for the actual changes
// 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 () => { | ||
if (cur.current.url !== url || pending.current !== p) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than synchronously checking whether the url has changed we need to do this within the effect, the url and mutative ref could be equal again in a scenario where we prerender --> lazy-load --> switch
which would mean that we push the oldChildren back up
I tested this PR's branch. I am afraid I can reproduce the bug consistently. The lazy-loaded component still appears below non-lazy routes, whenever I rapidly navigate from a lazy to a non-lazy route. Just to be clear: this is with production builds ( Also note that if the hydration is allowed to complete (in other words: no clicking on one of the header links to interrupt the lazy route), the I hope you will get to the root of this problem :) (you guys rock, by the way ... keep up the good work, please ;) |
@JoviDeCroock I will push my take on this to a draft PR so we can compare. Needs tests. |
@JoviDeCroock that's an interesting / informative development, I think: |
By removing the synchronous render work we remove a few race conditions that were assumed to never occur, in this case when a pre-rendered route was used and we navigated away from it, the pre-rendered route would come back later and re-insert itself as the first child.
Fixes #425
CC @danielweck in case you have a mo, could you test this 😅 been doing a lot of testing but our tests don't seem to agree with me, mainly wondering if I'm missing something important