-
-
Notifications
You must be signed in to change notification settings - Fork 110
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
Simplify Router's async handling and fix stacked transitions #530
Conversation
🦋 Changeset detectedLatest commit: a7e79c9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Size Change: +569 B (0%) Total Size: 734 kB
ℹ️ View Unchanged
|
Hello, I tested this PR's branch. As usual, I added the
Screenshot 1Screenshot(s) 2Screenshot(s) 3 |
Additional interesting fact: the "home" and "error" routes DO NOT trigger the |
Another very interesting fact: as I said in the previous comment, the "home" and "error" routes DO NOT trigger the |
@danielweck where are you adding the ErrorBoundary? It seems like that is breaking the suspend model here - ErrorBoundary intercepts suspensions that need to be propagated to Router for this to work. |
I'm not adding it anywhere, it's already here :) wmr/examples/demo/public/index.tsx Lines 25 to 37 in b3cb874
I just add the |
Alright, I've pushed a version that works in all these cases. |
Will test asap (11pm here ... and I've got a deadline to meet yesterday so.... ;) |
I tested the official WMR demo following these steps: ...and everything seems to work fine :) (I am now testing my own codebase, after disabling my workarounds ...) |
Alright, so first of all, well done for fixing this complex bug 👍 I still have to work around the following two issues in my particular usage scenario, but perhaps this is concerning in the general case too (I will need to file a separate issue, to document a repro pattern based on WMR's
|
Ah, I noticed this new "bug" (or feature?) following your recent code changes: |
Oh no, loads of errors in dev mode! :(
Hard-refresh the lazy route: ...Or hard-refresh any non-lazy route: |
Oh - sorry @danielweck I should have clarified: the RE The first bug you described is one I'm having trouble reproducing - are you setting state "above" the Router, or within a route? The not-yet-loaded route should be in a suspended status that causes any state updates to be dropped. I'm not sure about setting state above the Router - the correct behavior here would be for Preact to process the state update, re-render the Router, and bail out of progressive hydration (making it the same as the next thing:) The second bug is working-as-designed. When a route is pending, we attempt to render it one or more times in order to check whether it can be un-suspended. In doing so, we partially mutate its DOM tree - during hydration that's the actual DOM tree, whereas during client-side routing that's a work-in-progress disconnected DOM tree. When a different route is requested and we have to abort a pending async route, that DOM tree becomes invalid. Since it may be the current (partially-hydrated) tree, we have to destroy it to prevent the newly requested route from being forced into hydration mode. I do think that it might be possible to split out the "abort hydration of pending route and render something else" case from the "abort client-side transition due to new client-side transition" case. If we can do that, then the only time you'd get the blank page effect would be when clicking a link to a new route during the period of time when an async route is pending hydration. |
Thank you for taking the time to explain the underlying logic, much appreciated. Regarding |
Yes, above the router. That's probably an important distinction indeed! (and a bad design idea on my part?) Anyway, I have since moved to a different solution that doesn't involve local state at that level of the component tree. |
Are you able to reproduce the broken dev server / prefresh mode? ( Unless I am mistaken: I used |
@danielweck I haven't been able to repro yet but I'll give it a go. ErrorBoundary doesn't need to be removed, just wasn't sure if you were re-rendering via setState in response to |
Thank you :)
I was actually re-rendering via |
Alright, so the source of the error is undefined wmr/packages/preact-iso/lazy.js Lines 16 to 33 in a7e79c9
I'll fix this in my local branch to see if it helps. |
I have reverted to the |
@danielweck we don't know yet - as best as I've been able to tell, the linked PR doesn't preserve DOM during routing. The broken dev server thing is fixable, I'm pretty sure it's just a plugin (possibly prefresh?) forgetting to forward the third argument to |
Good spot, adding preactjs/prefresh#325 EDIT: published as 1.3.2 |
lol!! you beat me to that :o |
Awesome quick turnaround, you guys rock! :) |
I think I managed to make this decently readable!