Uh oh!
There was an error while loading. Please reload this page.
fix(react): Support lazy-loaded routes and components. - #15039
Conversation
| }, | ||
| { | ||
| element: ( | ||
| <Suspense fallback={<div>Loading...</div>}> |
There was a problem hiding this comment.
No strong feelings, this is also fine, but could we possibly add this to an existing e2e test app? Would save a little but of ci/processing time 😅
There was a problem hiding this comment.
We could add this to react-create-browser-router :)
| version, | ||
| basename, | ||
| // Use requestAnimationFrame to wait for Suspense boundaries to settle | ||
| requestAnimationFrame(() => { |
There was a problem hiding this comment.
this will slightly skew all timestamps, even if there is no suspense etc. happening, right? As this will realistically add ~20ms or so before handleNavigation() is called.
Not a blocker IMHO, but something to consider. Can we make this smarter (e.g. know when this is lazy?) somehow, possibly...?
| version, | ||
| basename, | ||
| }); | ||
| }, 100); |
There was a problem hiding this comment.
100ms delay seems quite a lot, this will skew stuff pretty considerably, I guess 😬 does that not add 100ms to every navigation duration etc...?
There was a problem hiding this comment.
I think we could maybe look into other fields of the RouterState https://github.com/remix-run/react-router/blob/d0e474cf6c521881044c445b4730c1a43aa77679/packages/react-router/lib/router/router.ts#L276
E.g. call handleNavigation when state.navigation !== 'loading'?
There was a problem hiding this comment.
Yes, updated with the navigation state checker 👍 I also realized that putting the manual timeout made us lose the inner resource.
❌ 1 Tests Failed:
View the full list of 1 ❄️ flaky tests
To view more test analytics, go to the Test Analytics Dashboard |
| }); | ||
| // Wait for the next render if loading an unsettled route | ||
| if (state.navigation.state !== 'idle') { | ||
| requestAnimationFrame(() => { |
There was a problem hiding this comment.
Oh, my idea with requestAnimationFrame actually worked - that's nice ✨
Thanks for implementing 🙌
But as Francesco pointed out - we should keep in mind the slight overhead and maybe there's even another way to implement this 🤔 Do we know if a route is lazy?
| }, | ||
| { | ||
| element: ( | ||
| <Suspense fallback={<div>Loading...</div>}> |
There was a problem hiding this comment.
We could add this to react-create-browser-router :)
size-limit report 📦
|
ea60f3a to
2a8888dCompareonurtemizkan
commented
Jan 21, 2025
@mydea, @s1gr1d, @chargome - I updated the PR to cover the lazy-loaded Routes too. So the problem was the cross-usage of We're actually updating Still, I think we can keep |
mydea
left a comment
There was a problem hiding this comment.
nice, this looks neat - great work!
Fixes: #15027 This PR adds support for lazily loaded components and routes inside `Suspend` on react-router pageloads / navigations.
Fixes: #15027
This PR adds support for lazily loaded components and routes inside
Suspendon react-router pageloads / navigations.