-
Notifications
You must be signed in to change notification settings - Fork 47k
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 react-server-dom-vite impl and fixture #26926
Conversation
Hi @nksaraf! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
packages/react-server-dom-vite/src/ReactFlightClientConfigViteBundler.js
Outdated
Show resolved
Hide resolved
handleHotUpdate({file}) { | ||
// clear vite module cache so when its imported again, we will | ||
// get the new version | ||
globalThis.__vite_module_cache__.delete(file); |
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.
@gnoff is working on a general solution to this problem as it’s a problem in the Webpack impl too atm. Next.js works around it.
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.
What kind of general solution are we thinking about? Do I need to do something to prepare for that?
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.
React's fast refresh currently runs synchronously but it should probably be a transition. In that transition application code needs to be able to run. For instance refreshing the router. We may need a way to provide modules in a transition compatible way.
I don't yet know what the eventual solution will look like just that there will likely be some API to hook into a refresh and maybe provide a new module cache or something like component families but for modules
You don't need to do anything now to prepare per se, but when I get started on it I'll reach out to figure out what needs to change
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.
It turns out that I think we probably don't need that approach after #26985 since we have another way to avoid the cache.
One possible solution is to use the metadata object as a key (or just stashing the value on the metadata) because that object is unique to an RSC request. It means it'll go through the async pass during new RSC loads even for existing modules but that's only during the preload phase and not rendering.
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'm not sure I get why this is needed for Vite though since Vite adds a timestamp for the new version of the module. If that timestamp is not used by the RSC loader, I suspect that leads to other issues so this might indicate another bug.
For context and credit. There was some previous work done on a Vite plugin from @frandiox in #22952 which powered Hydrogen. That was done before we ratified the react-server conventions and ironed some details which is why it wasn't merged - since it wasn't following the new convention. This new take seems to address those by building on top of the latest stack. |
Yea, I'm not too happy about how that turned out. I expect us to do a few more iterations there on the format.
The essence is that we support suspending on the CSS way later than you typically would in a JS module loader. E.g. you'd have to download the CSS before loading the JS. In our Webpack set up and Next.js we can load CSS in parallel while still rendering the next page that needs it. While it's more of a meta-framework concern, it could be good to show how that's intended to work in a meta-framework. E.g. the Webpack fixture still collects the CSS for the entry point and loads it using React at the root. That could all go into the fixture though. We don't really have an equivalent for the plain ESM fixture since there's not a solution to this problem in the pure ESM world that also works on the server. It's also less efficient due to the lack of parallelization. |
I want to give some credit too for all the different references I had to build this up including some direct work from @cyco130 when we worked on cyco130/vite-rsc together
Cool will add the CSS support in the fixture (had that in our experiments). I think for the module reference format I will revert to |
@sebmarkbage added support for CSS files in the fixture. It uses an Async server component to load the css files imported in the module tree under an entry. During production, it uses the manifest to figure out what css files to include. Also now using the |
Blocked by vitejs/vite#13487. Right now vite is not respecting user config's |
For dev and prod, React already builds two different versions so you can have different implementation details for each one. Another question that came up for Turbopack was whether we should have a 2x2 matrix of HMR/build bundler and development/production mode React, or if it's enough to just have production/development. |
|
||
function createFromNodeStream<T>( | ||
stream: Readable, | ||
moduleMap: string, |
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.
Agreed. I think I hold the same viewpoint. The trouble I had was in making the CJS choice a strict opinion and the abstraction started bending there. I think we would need to allow both ESM and CJS output while bundling, and so the runtime needs to allow the consumer to pick which one to use. The other issue was between the browser, edge and node clients. Only the node client will be able to support CJS require based sync initialization. The other would either need to bundle everything together and then just access the relevant thing syncronously. Or have async require using dynamic imports. Does the react team have an opinion in that space too? |
There's three parts to the synchronous nature of increasing importance:
Each platform should implements as many of these as possible. For the Webpack build we solved all four. For the ESM build I only solved 2 and 4 by using a shared global cache but not 1 and 3. Because it's the best we can do given the underlying system. The Vite version should be able to support at least 2, 3 and 4 in production. |
It doesn't look like you have a solution to 3 here. I suspect you'll need to have Vite generate a manifest that includes the recursive generated files you'll need for every client module. |
Yeah not yet. But yeah I can get it to emit a manifest for client modules and what other client imports it has. This should allow us to do 3. But also if we are doing synchronous loading, using CJS, does this chunks thing matter still?
|
The chunks thing is a prerequisite for doing sync loading since you need to guarantee that you already have all the chunks loaded. |
We don't have strong recommendation for Node builds. There's different tradeoffs depending on if you use a warm server or cold starts. It's also less sensitive because the downstream effect of a few extra microtasks is much lower in the SSR implementation. So I'd say that's not strictly necessary. Sync init can help cold starts on the server but it's not quite as impactful as on the client. So if you're not going to do it on the client, probably not worth it. |
Could you help me out with which ones of these needs to apply to the SSR or the browser or both? Syncronous loading on the client is probably more difficult problem. Will probably need to chunk things better and load those before the hydrating begins. Is that the idea? |
They're all relevant for both but more so on the client. For SSR it also depends on how you deploy. E.g. if you bundle the whole thing into one bundle then there might not be any chunks to load and so it just never will have an extra chunks. |
Haven't been updating this PR for a bit to actually explore how to do this kind of sync bundling in vite/rollup. Will probably get some help from the team there but right now going han solo to figure out how everything works. |
@nksaraf Sorry if I misunderstood anything but doesn't Vite already do that automatically in production? Have a look here:
Doesn't this solve |
Yeah I think you are right, to use this feature we would have to create virtual modules that were dynamically importing the client components and using that for loading.. right now I was looking at a more runtime driven approach using a manifest so that I can import things at will during runtime without worrying. But I think I'm gonna explore how far using bite's default behavior can take us |
Cool. All client components still need to be included in the client build somehow, either by naming them explicitly as entry points or using a virtual module like you mentioned. A virtual module (which would essentially serve as a client components manifest) would solve both problems so I thought it might be the easiest way. Especially since Vite would do all the heavy lifting for you automatically and would do it at build-time which is probably more efficient. |
Let me know if you need help rebasing since a lot has changed in the internals. |
Yeah would love the help |
@sebmarkbage this sounds like a very good reasoning for me why module loading should be the consumers concern to implement and let the consumer decide on which tradeoff to choose. Vite and esm is fully on the async module loading train, so I don't see the point in taking commonjs and sync module loading into consideration. When someone chose Vite and esm, then async module loading was already chosen as preferred by the developer/consumer. Is there any stress test or similar for benchmarking performance using sync vs async module loading? So there would be a measurable score for decision making. It would be nice to have benchmarking for webpack vs turbopack vs esm vs vite versions of react-server-dom too and some info on what would be the tradeoff when the module loader would be provided by the consumer in a generic solution.
@nksaraf after module caching was removed from react-server-dom-webpack, I needed to implement it again the same way as it was before in the mock In a production build I'm using the manifest files generated by Vite for the client and server builds. I also use the manifest to collect all the CSS files needed and render links for each. If any of you are curious what I mean, all my work on an experimental framework using Vite is at https://github.com/lazarv/react-server. |
Hey 👋 Thanks for working on this. Just wanted to say that at Stormkit we develop and maintain a React Monorepo Template which is framework agnostic. It uses Vite to build and run the app on development. It'd be super useful to have Vite support for Server Components. Therefore just wanted to say that we're ready to help if there is anything we can do to speed up the support for this feature 🙏 Once again thanks a lot! |
@sebmarkbage should I just start from scratch. Seems like it'll be easier to do it that way than tracking what all has changed in other versions |
Might be necessary given that vite 5 is also on the horizon? Sounds like a tonne of work though 😟 |
Hi there 👋 Happy to know that there is a work which is done to support RSCs in Vite. I'm building my own Framework on top of Vite actually and I really need to support that feature too. Hope that all will go well and we will have something working fine. Good work guys. |
This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated. |
Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you! |
Could you please consider to either:
|
Summary
Vite is one of the most popular bundlers used to build React apps. This PR is adding a vite integration for React server components. It would be shame if the usage of React server components is restricted to webpack or Next.js's bundler.
The current experiments that use vite with React server components, all hack into the
react-server-dom-webpack
package and shim the environment that webpack expects, specifically__webpack_chunk_load__
and__webpack_require__
.While it is okay as a starting point, it would be good to get a deeper integration with vite that recognizes vite's lazy by default approach to doing work, and its ESM first nature. One big issue with the webpack integration is HMR. Right now because it caches the modules it loads with no way to invalidate that cache, performing HMR in vite's style requires some stupid hacks. Ideally this should be handled properly by exposing a way to invalidate the module cache. We do this by exposing
__vite_module_cache__
. We also expose a__vite_require__
function for the integration, (similar to__webpack_require__
). This allows the user to specify how the module loading should behave between dev and prod.Technical Details
SSR Server (
global
): Uses vite to serve client assets and transformed ESM modules to the browser. Also uses vite to load client components for SSR. We setup the environment with__vite_require__
and__vite_module_cache__
which uses vite during development and the built manifests during production.React Server (
region
): Uses vite to load the app entrysrc/App.jsx
with the appropriate resolve conditions ("react-server"). It also uses vite to load the server action module when called by the client. We setup the environment with__vite_require__
and__vite_module_cache__
which uses vite during development and the built manifests during production.A few aspects left to clean up:
module/path#export
. I tried saving them as["module/path","export"]
. But it looks like with form actions it treats the string as the ID and so it doesn't work well with the array representation. So might have to change it all to just use the oldmodule/path#export
format.link
tags for those. Should be able to be handled at a meta-framework level.How did you test this change?
Added a fixture
flight-vite
that's modelled after theflight-esm
andflight-webpack
fixtures. During development, theglobal
andregion
servers use vite to parse, load and transform the modules for SSR and creating an ESM dev server for the browser. A build script is added to use vite to build the react server, SSR server and the client bundles. In production, usingyarn start
, theglobal
andregion
servers use the manifests produced by vite to know where to load the bundled modules from for the react-server, SSR and browser.