-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Add support for clientLoader + Fallback #8092
Add support for clientLoader + Fallback #8092
Conversation
🦋 Changeset detectedLatest commit: aed8736 The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
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 |
I understand that this Fallback component is kinda like the old Pending export RFC? What happens if I export a |
@sergiodxa Thanks for cross-linking that. Right now This same "navigate immediately" idea occurred to me though so I'm chatting with @ryanflorence on whether that's something we want to consider supporting as part of this work or something best kept as a separate undertaking. To some extent though, |
integration/client-data-test.ts
Outdated
// Full SSR - normal Remix behavior due to lack of Fallback components | ||
await app.goto("/parent/child"); | ||
let html = await app.getHtml("main"); | ||
expect(html).toMatch("Parent Component"); | ||
expect(html).toMatch("Parent Server Loader"); | ||
expect(html).toMatch("Child Component"); | ||
expect(html).toMatch("Child Server Loader"); |
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.
In this case, I understand the flow will be
- SSR with server loaders
- Hydration with server data
And client loader will be ignored until the user navigates back to this route client-side only?
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.
yeah - clientLoader
's are always called on SPA navigations. They are skipped on initial hydration by default, and called on hydration when a Fallback
exists.
integration/client-data-test.ts
Outdated
let app = new PlaywrightFixture(appFixture, page); | ||
|
||
// Renders parent fallback on initial render and calls parent clientLoader | ||
// Does not call child clientLoader due to lack of Fallback |
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.
I think a parent route fallback should enable child route client loaders to run, because the fallback can wait for any nested route client loader too.
Similar to how Suspense works, if I have:
<Suspense fallback={<Fallback />}>
<ParentRoute>
<ChildRoute />
</ParentRoute>
</Suspense>
The Suspense will render the fallback until both ParentRoute and ChildRoute renders.
But thanks to <Outlet />
Remix can enable the ChildRoute to render inside the ParentRoute fallback component too, similar to this:
let child = (
<Suspense fallback={<ChildFallback />}>
<ChildRoute />
</Suspense>
)
<Suspense fallback={<ParentFallback>{child}</ParentFallback>}>
<ParentRoute>
{child}
</ParentRoute>
</Suspense>
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.
yeah I've gone back and forth on this - planning to talk it over with @mjackson and @ryanflorence. On one hand I really like the fact that the current implementation allows me to answer the "does this run on hydration" question from the same file. But what you are proposing is more in line with Suspense concepts - but it makes it a bit harder to track down since now you need to walk up the route hierarchy yourself.
I also don't want to introduce uneccesary calls - for example if a child clientLoader
doesn't want to run on hydration but some parent does - then we'd call the child clientLoader
for no reason. But we are planning to provide a way to know in the clientLoader
if it's the hydration call or not (currently via a custom header) - so you could short circuit manually:
function clientLoader({ request, serverLoader }) {
if (isHydration(request)) {
return serverLoader();
}
// logic for subsequent SPA navigations
}
Has there been any discussion regarding HMR feature with remix/packages/remix-dev/vite/plugin.ts Lines 1088 to 1090 in 787bb83
There is a corresponding test case here and It might be interesting to see what happens if there are remix/integration/vite-dev-test.ts Lines 200 to 229 in 787bb83
|
clientLoader.hydrate=true | ||
export function HydrateFallback() { | ||
return <p>Parent Fallback</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.
Is the fallback going to be called HydrateFallback
? Also we will need to add clientLoader.hydrate = true
to every clientLoader we want to run on hydration?
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.
Is the fallback going to be called HydrateFallback
Yeah, we went with HydrateFallback
for a few reasons:
- It's only relevant for initial document hydration when you opt-into the
clientLoader
executing on hydration (viaclientLoader.hydrate=true
) Fallback
/ClientFallback
are misleading because they might imply that it will render during subsequent SPA navs which is not the case- The verbiage aligns with
clientLoader.hydrate
andReactDOM.hydrateRoot
Also we will need to add clientLoader.hydrate = true to every clientLoader we want to run on hydration?
Yeah:
- The default behavior matches the current remix behavior. Every route has a client loader in Remix today which is just the
fetch()
call - and those do not run on hydration because we already have the data. Exporting aclientLoader
is basically you taking more control over this existing client side loader - but it does not change the behavior. - We believe
clientLoader
as an optimization is the more frequent use case (basically where clientLoader doesn't change the shape of the returned data)- Client side caching
- Hitting an API directly on a SPA nav and bypassing the Remix BFF network hop
We think the use case where you want to run on hydration is likely less frequent since it eliminates your ability to SSR content, introduces more potential for CLS, etc.
Question, let's say I define a clientLoader in |
Another question, regarding TS, should we do |
|
||
// Throw an error if a clientLoader tries to call a serverLoader that doesn't exist | ||
if (initialData === undefined) { | ||
throw new Error( |
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.
Is this something we want to throw on or something we want to just return null / undefined as if there wasn't a loader? Reason this stands out to me is because it makes it seem like implementing shared behavior could become more difficult / leak implementation details.
If I wanted to have a route template that ensured a clientLoader that dealt with a global caching strategy was exported by default from all new routes, they would also have to have a loader that just returned null or the route wouldn't work until one was added.
Does that sound right?
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.
I considered this too and almost removed it since right now we just return null
in fetchServerLoader
. The thing that felt maybe slightly different was in the current case, it's the framework calling it and we have to have a loader to handle the JS/CSS module loads. In the new flow, it's the user calling a function erroneously so it felt maybe more confusing to let it fail silently.
I could absolutely be convinced to remove this error path though - curious what @mjackson and @ryanflorence think
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.
IMO if I call a route serverLoader but I don't have one, it should throw so I either catch it or the error bubbles and got reported to my Sentry
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.
yeah it's essentially the same as if you manually did fetch('/route-without-loader?_data=routes%2Froute-without-loader')
which would throw
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.
err....scratch that. That doesn't throw but returns a 400 with You made a GET request to "/" but did not provide a
loader for route "routes/_index", so there is no way to handle the request
.
Insert debate here on whether fetch should throw on non-200 responses 😂
@jacob-ebey and I talked about this too. Right now, If you have use-cases where you need to render deeper but you also want to get some client data in the root UI - I think I'd just grab that data synchronously in the root |
Depends on the use case:
|
df152c2
to
3595819
Compare
31ce990
to
28e8d31
Compare
28e8d31
to
aed8736
Compare
Now that I think we're mostly feature complete in this series of PRs, I'm going to get this merged into the |
New Feature Branch PR: #8173 |
Depends on #8090
This will fail CI until we pin it to a React Router experimental release with the work in remix-run/react-router#11033This is the third in a series of PRs to implement Client Data into a
feature/client-data
branch.This PR adds support for
clientLoader
(which is the more complex of the two sinceclientAction
doesn't need to worry about hydration flows).clientLoader
by itself, we will not run theclientLoader
on initial hydrationclientLoader
and setsclientLoader.hydrate=true
, then we will call theclientLoader
on hydration and display the deepest availableHydrateFallback
Todo: