-
Notifications
You must be signed in to change notification settings - Fork 324
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
Combine SSR and RSC responses + Request Provider #390
Conversation
packages/hydrogen/package.json
Outdated
"react-client": "link:../../../../react/build/node_modules/react-client", | ||
"react-server": "link:../../../../react/build/node_modules/react-server", | ||
"react-client": "file:../../../../react/build/node_modules/react-client", | ||
"react-server": "file:../../../../react/build/node_modules/react-server", |
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.
This is actually important because having symlinks means that we end up using 2 different versions of React. This makes everything fail because React dispatcher gets confused.
const {PassThrough} = await import('stream'); | ||
const writer = new PassThrough(); |
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.
For some reason, the HydrationWriter
we use in main
branch doesn't get the whole RSC response here, only part of it. PassThrough
or a real ServerResponse
gets all the RSC lines...
return cached; | ||
} | ||
|
||
console.log('---FETCHING', key); |
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.
Notice that this is only logged once per request (per key) since both SSR and RSC use the same cache.
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 love this idea. unstable_getCacheForType
seems like just what we need. We should reach out to the FB workgroup about using this for RSC.
packages/hydrogen/src/foundation/ServerRequestProvider/ServerRequestProvider.tsx
Outdated
Show resolved
Hide resolved
packages/hydrogen/src/foundation/ServerRequestProvider/ServerRequestProvider.tsx
Show resolved
Hide resolved
packages/hydrogen/src/foundation/ServerRequestProvider/ServerRequestProvider.tsx
Show resolved
Hide resolved
As mentioned in the OP, there's an issue with the SSR part where the context of function doRequest() { return fetch('http://localhost:3000', {headers: {accept:'text/html'}}) }
await Promise.all([doRequest(), doRequest()]) I've been debugging this but I can't find why it happens. The same strategy currently works in @wizardlyhel Do you see any difference between your |
const requestCache = React.unstable_getCacheForType( | ||
requestHydrationCache | ||
); |
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 don't think this is where we should be using unstable_getCacheForType
.. maybe inside useServerRequest
is where it should be.
Any cache instance that is created in the entry file is going to be singular instance to the life of this worker. Since the entry file is managing multiple requests, when there are 2 requests under the same key happening around the same time, the latter request will clear the cache of the previous cache.
I think what you want to achieve is 1 cache instance per rendering request and only lives for the duration of the rendering request, is that 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.
As far as I understand, the cache used in unstable_getCacheForType
gets instantiated once when rscRenderToPipeableStream
is called.
Since each request calls this function separately once, I think we end up with different caches per request even within the same worker. Am I wrong? 🤔
Assuming this works, then we place the request
object in this cache so it can be retrieved later in other parts of the tree.
The same request
object is passed later to the SSR rendering as Context/Provider (this is where I find issues with multiple requests at the same time).
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.
From the sounds of it, it doesn't looks like unstable_getCacheForType
is behaving what we think should be happening based on the issue you described.
My exploration of RenderCache
stuff starts with an empty {}
and gets filled by the end of a render.
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.
@wizardlyhel But the problem occurs in the SSR rendering where unstable_getCacheForType
is not used. In fact, I was testing and this same problem happens in main
branch with RenderCacheProvider
.
The context gets reset to the initial value but in RenderCacheProvider
that's an empty cache, so it doesn't throw but probably returns undefined values. If you initialize that context to null
you will start seeing No RenderCache Context found
errors. Not that this only happens when rendering 2 requests at the same time:
function doRequest() { return fetch('http://localhost:3000', {headers: {accept:'text/html'}}) }
await Promise.all([doRequest(), doRequest()])
So it looks like this is a bigger issue that what I thought, and it's already in main
branch :(
Will report it properly tomorrow in a new issue.
This is super cool by the way - Just got it running on my local machine and looking on that double request issue |
This is very interesting - the RSC flight injection is happening before the body streams. I'm gonna play around a bit and see how this behaves by putting a couple suspense boundaries. |
This is awesome! The main html body contains the static html when more suspense boundaries are placed. With this, we don't really need to worry about which (RSC or stream blocks) get injected first. Here is the change I did in
|
@wizardlyhel Thanks a lot for testing it! Glad it works 😅 |
Description
Attempt to implement #250
This PR is an exploration on how to implement Server-only context that is supported by the official RSC and how to combine SSR+RSC responses into 1 (for performance). Feedback is very welcome.
This is using the official RSC just like in #317 and adding a few extra features:
request.context
object where we can store everything related to the current request.request.context
) to the React tree.React.unstable_getCacheType
to create some kind of cache that is scoped to the current RSC rendering (i.e. it doesn't conflict with other requests). In this cache, it stores the current request so it can be retrieved from anywhere in the React tree (just like a context provider, but works in RSC).useServerRequest
hook that gets the current request anywhere in the tree (only callable from server components).So far, we have Server-only Context to store data that is scoped to the current request, both in SSR and RSC, and have it available anywhere in the React tree. Examples of things to store here are: fetch promises (just like
RenderCacheProvider
inmain
branch); logger that shows the current request data; etc.Finally:
request.context.cache
for fetch promises. This means it generates both responses at the same time but without repeating subrequests. When the SSR is done, it attaches the RSC response to the HTML and uses it in the browser instead of making a new RSC request.Things not implemented
Every
rscChunk
is one line of the flight response so it can be consumed in the browser to prefetch client components asap.Additional Context
Follow #317 indications to try this locally.
I'm not 100% if this all works well yet for bigger apps (it should?). There is a problem when having 2 tabs open (2 requests at the same time) related to the SSR context/provider (the context gets lost), but the RSC side works well. Not sure if I'm using Context/Provider correctly.