-
-
Notifications
You must be signed in to change notification settings - Fork 26.9k
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
SSR support #6747
SSR support #6747
Conversation
Thanks for the PR! This looks promising. We're 100% focused on getting the 3.0 release out right now but as soon as that's done we'll take a closer look at this. |
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.
@jamesknelson, I'd love to see us get this in ASAP. We'll need to look at it as a 3.x release as @iansu mentioned, but I've just had a quick look over this and it looks great.
Glad to hear that :-) About the test suite, it seems to be failing due to filenames in error messages being prefixed by - ./src/App.js
+ (undefined) ./src/App.js
Line 3: 'foo' is defined but never used no-unused-vars I'm pretty sure that the reason for this is that the PR adds source map support within the dev server, so that we can get proper stack traces for the server rendering part: require('source-map-support').install() Eventually I think it'd be better to pass stack traces and source maps for errors from Will it be okay to just regenerate the snapshots and leave the |
@@ -105,7 +122,7 @@ checkBrowsers(paths.appPath, isInteractive) | |||
// Create a webpack compiler that is configured with custom messages. | |||
const compiler = createCompiler({ | |||
appName, | |||
config, | |||
config: [webConfig].concat(paths.appNodeIndexJs ? [nodeConfig] : []), |
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.
// maybe easier to avoid unnecessary concat
config: paths.appNodeIndexJs ? [webConfig, nodeConfig] : [webConfig],
I went down this route in my own ssr-enabled fork and ended up not using the multicompiler in normal day-to-day development. I was having issues where my large app was taking a very long time to recompile. Doing start
in client-only mode was super fast but multicompiler was slow (more than a minute to rebuild). I ended up adding a custom watch.js
script that allowed me to watch the client and server code in separate processes, which was super fast.
I would suggest testing this on a large app to see if you're having the same issue.
You will also want to fix the babel config in the Generally you will want to target commonjs when building for node :) |
any timeframe for this? |
For what it's worth, I've been using this fork (via the universal-react-scripts package) in a medium sized app, and build performance has been great. I have not run into the issues mentioned by @heygrady. I still get ~instant rebuilds in an app with ~25 routes and many more components, and that's with styled-components and all the extra weight added by needing to render the page to HTML after each save. The whole development flow is actually pretty nice. It really just feels like create-react-app – you don't even need to think about SSR 95% of the time. So I guess what I'm saying is that we'd need to add tests before any merge, but it all seems to work pretty well from my experience so far. |
Thats awesome! Do you have any documentation on how to upgrade to your version? Or when do we expect the tests to be fully completed? |
@alexvandervegt Big disclaimer here: I won't promise any support for this fork into the future. But with that said, there's some documentation in the README on this branch: https://github.com/jamesknelson/create-react-app/tree/universal-react-scripts Re: tests, I'm not sure when I'll be able to find the time. If there's still interest amongst the maintainers in merging this, tests may be something you could take a look at. But I'd wait for confirmation from @iansu before doing any work. |
Do you use your code with react-router v.5.0? Because i get this error: An error occured while server-rendering your app: Error: Invariant failed: You should not use outside a ===> import {StaticRouter as Router} from 'react-router-dom'; |
I’m using Navi, but yeah, you’ll need to use a StaticRouter in index.node.js and a normal router in index.js if you’re using react-router. |
I got it all working now and its awesome. Only thing I can not seem to get working is the react-loadable on the server. How did you manage this? |
I'm not using react-loadable, as Navi handles the same thing in my setup. If you can get it working though, it'd be great to see how you handle it! |
I need to add a plugin to webpack.config but I dont want to eject the react-scripts. If you know a way how to handle it I can make it work and post it here. |
Can you give me your example with Navi? |
@alexvandervegt it might be best to move this conversation over to the issues on my forked repository, so that we're not spamming everyone with notifications. You're right in that node does not currently support ES2015 modules, but this shouldn't be an issue so long as you only import commonjs format packages. As for an example with Navi, it's coming but not quite ready. |
For future reference, there's been some interesting discussion around this on a reddit thread today. After reading through it and using this PR personally for a couple months, my gut feeling is that SSR support can be a lot simpler than existing solutions make it out to be. The major difference with this approach is that it treats routing, data fetching and the server itself as application code, so it doesn't need to make any decisions about them. All it needs to do is provide a way to build a commonjs module, and make some decisions about the format of the exported middleware (so that rerender-on-file-save works). This then gives people the freedom to use whatever tools they want, and importantly, it gives people the freedom to come up with new tools as React itself evolves (e.g. with the streaming renderer). For example, it lets you use react-router for routing, Apollo for data fetching, and Express for the server on one hand -- or universal-router, Firebase Functions and raw Suspense for data fetching (once it's ready). Either way works great, because all this PR takes care of is the build itself. The one issue I see with this design is that the rerender-on-file-save middleware needs to make some assumption about the commonjs module's exports. But given that everyone seems to support express style Request/Response objects, I don't think it would be too controversial to just say "if you want rerender-on-file-save, you need to export a |
@jamesknelson, would you be interested in helping to support this feature over the coming months if it gets added in? |
@mrmckeb Sure. Even if it doesn't get merged, I'm supporting cura anyway (already merged in and published the changes for 3.1.0). Would definitely prefer to support it here if possible. |
Co-Authored-By: Federico Zivolo <federico.zivolo@gmail.com>
@jamesknelson Any update ? I hope this gets official support and then I can use it. I am very excited. |
@jamesknelson Any update on this? Would love to start using it once it's officially supported :) |
I doubt this will be merged, given that the React team seem to be looking into server integration inside of React itself now. Some communication would have been appreciated instead of the "we want to merge this" and then radio silence, but I think it might be best to close this for now to prevent further confusion/wasted time. |
Sorry @jamesknelson, this obviously fell off of our radar - which was our fault 100%. As we're a very small team of volunteers, features like this add complexity that we don't feel we can currently support. Instead, we recommend using something like Next.js for SSR (or SSG), or Gatsby for SSG. There are obviously other great solutions available too. |
So why did you tell @jamesknelson you would have looked into it? |
We should never stop exploring ideas. Investing how SSG would fit into CRA was a good idea at some point but then we discovered things like complexity down the road. next.js and gatsby already are great alternatives. At this point its better for create-react-app focus on being an option for client side rendered apps only and we will se in the future if it changes depending on how react it self evolves. |
I see, the fact it took 2 years to "look into it" is worrying though. Don't expect the community to get involved if that's the respect you have for contributors' time. |
Hi @FezVrasta, when I said "I'd love to see us get this in", I meant that I would personally love to see SSR in CRA. I definitely should have worded it differently as we had only just started discussing it, and it wasn't meant as a commitment. Since that time we've discussed SSR in more detail, and as @andriijas said it proved to be more complicated than we first thought. Mainly, the issue is that we don't have a big enough team (or perhaps a team with enough time) to support the complexity it introduces.
Please remember that we're also part of the community. We all do this in our personal time, and we also contribute to other projects. We encourage contributors to raise PRs only after we discuss new features in issues, as we never want anyone to spend time on something that doesn't end up being merged. It was a failure on my side that I didn't close the communication loop on this topic, and I'm sincerely sorry that time was wasted and really appreciate the effort that @jamesknelson put into this. |
Let me add my 2c here. Razzle is another great alternative to support SSR and it gives the same experience as CRA and supports React Router. https://github.com/jaredpalmer/razzle. It took me no time to move my CRA-based app to Razzle. |
This adds optional SSR support for create-react-app.
It takes a similar approach to TypeScript support -- the existence of a
src/index.node.js
file enables SSR support. Whensrc/index.node.js
does not exist, the behavior does not change compared to the current master branch.I haven't yet added docs/tests, but you can try it out following the instructions below. If there's any chance of this being merged, I'm happy to add tests/docs -- but I'd like to see some discussion before doing the work.
This PR does not attempt to solve routing or code splitting, as streaming SSR should eventually remove the need for anything extra for handling these, and it's still possible to handle these right now using things like Apollo, react-ssr-prepass or Navi.
How to try it
Alternatively, I've published a version of this to npm as
universal-react-scripts
, so you can try it out using the standardcreate-react-app
script:The node bundle
When
src/index.node.js
does exist, a Node version of the bundle will be built alongside the Web version, usingsrc/index.node.js
as the entry point instead ofsrc/index.js
. Here's my proposed default:When this file exists, two bundles of the app will be built: one in
build/web
, and one inbuild/node
. The version inbuild/node
is a commonjs module, so you can do anything with it, but the above default has been chose to be easily usable from Express, and from Lambda function on services such as ZEIT Now and Firebase. It also allows for SSR during development, using the dev middleware.The dev middleware
This adds a new middleware that is enabled when
src/index.node.js
exists. This middleware loads the default export ofsrc/index.node.js
, and uses it to render each page during development -- ensuring that server rendering "just works" during development. This should cause no user-facing changes at all.Other changes
.web.js
extensions, I've added support for.node.js
extensions as well, which are given preference for the node build.process.env
to be used within the Node files, I've modified env.js to pass through any existing value of process.env.--universal
option is passed through fromcreate-react-app
to the init script.[templatename]-universal
template which can be used to override specific files. This is used to add thesrc/index.node.js
orsrc/index.node.tsx
file when--universal
is enabled, and to switch toReactDOM.hydrate()
instead ofReactDOM.render()
.