Skip to content
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

server-side render POC #172

Closed
wants to merge 1 commit into from
Closed

server-side render POC #172

wants to merge 1 commit into from

Conversation

tomkis
Copy link

@tomkis tomkis commented Jul 25, 2016

This is just a throwaway code not meant to be ever merged

This PR is just a proof of concept how server side render could be implemented. I am aware that it's quite opinionated and introduces new dependency on express.

I am open to any discussion how much sane this would (or would not) be, I'll gladly re-implement it in production quality if we decided to go this way.

@@ -10,7 +10,6 @@
var path = require('path');
var autoprefixer = require('autoprefixer');
var webpack = require('webpack');
var HtmlWebpackPlugin = require('html-webpack-plugin');
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently HTML must be generated on the server.

@ghost
Copy link

ghost commented Jul 25, 2016

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

@@ -98,23 +95,6 @@ module.exports = {
return [autoprefixer];
},
plugins: [
new HtmlWebpackPlugin({
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The obvious drawback is that HtmlWebpackPlugin did some HTML postprocessing and most importantly it automatically added chunk hashes to imported files, which now needs to be done manually on the server.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can also combine HtmlWebpackPlugin and SSR, it is limiting, e.g. doesn't support streaming (since it's not rendered by react fully), but you can totally get access to generated HTML on the server

@ghost ghost added the CLA Signed label Jul 25, 2016
@ghost
Copy link

ghost commented Jul 25, 2016

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@gaearon
Copy link
Contributor

gaearon commented Jul 25, 2016

This looks very cool. We won’t get this in anytime soon because of the complexity, but I could imagine somebody wanting to maintain a fork that allows that.

@gaearon
Copy link
Contributor

gaearon commented Jul 25, 2016

Closing because this is not going to be actionable for us—props for doing it though!

@gaearon gaearon closed this Jul 25, 2016
@AlicanC
Copy link

AlicanC commented Aug 20, 2016

@gaearon weren't you planning to introduce optional features? Couldn't this be something like npm run add server-side-rendering? (Like npm run add flow or npm run add relay.)

@gaearon
Copy link
Contributor

gaearon commented Aug 27, 2016

No, we don’t plan to add optional features at this point.

@AlicanC
Copy link

AlicanC commented Aug 28, 2016

Then it should be core. Facebook's crawlers do not run JS so you need SSR for your OGP <meta>s. I believe that something made by Facebook should work well with Facebook.

And this is not a Facebook-specific problem. You might not see this since (I assume) you are at the developer side of things and not at the user side, but an app that doesn't support Open Graph, Twitter Cards or other <meta> stuff is completely unacceptable by customers. If I create a PHP, Rails or ASP.NET project right now, this won't be a problem for me. If I go for React, it will.

The README states that "[our] environment will have everything [we] need to build a modern React app", but it doesn't. An app that can't be displayed properly on social media is not a modern app.

@lacker
Copy link
Contributor

lacker commented Aug 28, 2016

I would love for server-side rendering to be easy to set up, extremely simple, and just a core React thing. Unfortunately that is not the case for server-side rendering today.

I think the way to get there is not to put a complex implementation into create-react-app, but to work on making it simpler to add SSR into a React app in general, with a small and simple library in the modular "do one thing well" sort of style. If there were a single very-popular SSR library, then it would be a lot easier to go convince Dan that we should support it in create-react-app. That's the situation with webpack, babel, and eslint, for example - they are each tools that became dominant in their area in the React community, so it became clear they would be a good fit create-react-app.

So here is a challenge for folks that would like server-side-rendering to be part of create-react-app. Could you make a new library so that, to make an app with server-side-rendering, all you have to do is:

  1. Run create-react-app
  2. Run npm install your-new-library --save
  3. Just add a few lines of code according to some simple instructions
  4. And now server-side-rendering works for you

@gaearon
Copy link
Contributor

gaearon commented Aug 31, 2016

Facebook's crawlers do not run JS so you need SSR for your OGP s.

And this is not a Facebook-specific problem. You might not see this since (I assume) you are at the developer side of things and not at the user side, but an app that doesn't support Open Graph, Twitter Cards or other stuff is completely unacceptable by customers. If I create a PHP, Rails or ASP.NET project right now, this won't be a problem for me. If I go for React, it will.

I believe there is some sort of a misunderstanding. Just because CRA gives you a client-side app, doesn’t mean you can’t add something to its output for the server side.

You can take the built index.html and add something like <!-- meta --> to it. In development, it would be ignored. For production, you can set up a build step that copies that index.html to your server’s template server. Then you can set up your server side code to take index.html template and replace <!-- meta --> with whatever information and tags you want to generate depending on the URL. You can even write that logic in JavaScript, or use the same router, if you want to.

There is absolutely nothing preventing you from adding <meta> support to React projects. You don’t have to render React on the server to support them 😉 .

@gaearon
Copy link
Contributor

gaearon commented Aug 31, 2016

Added instructions about <meta> tags in 9d9d31e.

@gaearon
Copy link
Contributor

gaearon commented Aug 31, 2016

To add to @lacker’s comment, here’s a bunch of non-obvious things you need to solve for good server rendering setup: https://react-server.io/. Yes, such solutions exist, but you can see that even that solution has a very opinionated API. This is because setting up server rendering correctly is a hard problem, and very hard to do in a generic way while maintaining beginner friendliness.

@AlicanC
Copy link

AlicanC commented Aug 31, 2016

We already came up with a similar workaround. It worked fine because the only route we had was /*. We would have to rewrite a lot of code for the server if that wasn't the case (and it wouldn't be if this wasn't a legacy app). We would like to avoid maintaining two copies if we can.

What we are looking for is a proper isomorphic (universal) solution to this. Sadly, the "solutions" available to the community today feel more like workarounds than actual solutions. If SSR is too complex then let's fix this. We can discuss what is needed to make it simpler and push vendors to improve the situation. The situation isn't going to improve if we admit defeat and stop challenging the stack. We should challenge it and not just one by one, but as a whole stack so everything fits together perfectly.

This project looks like the place to do it since it is official and also defines a stack, unlike React or Webpack which are just parts of stacks. I'm not suggesting we add everything to the project, I'm suggesting we add simple things and try to make the rest simpler. Not everything has to be in the example code. Just support for them have to be there so a developer can easily use them when their time comes.

@gaearon
Copy link
Contributor

gaearon commented Aug 31, 2016

The scope of this problem is much bigger than the scope of this project. We don't have a complete team supporting it full time which is what I believe would be necessary for the kind of project you are describing. It would be a noble effort and we'd be happy to link to it, but since we don't actively use React on the server in production, we are not the right people to build this. If your company is willing to invest that effort, I'm sure there will be plenty of interest.

@AlicanC
Copy link

AlicanC commented Sep 3, 2016

I know that you don't work on things that you don't need (like Windows support) and I absolutely understand, but is the line you draw really that thick? We trust Facebook and React, but every part of the stack that does not have support from FB is in flux and SSR is one of them.

Even FB does it in Hack, it still does SSR and surely has something to offer to JS land. I think even an intent to include SSR in this project would be enough help. You can analyse what makes SSR so complex, open necessary issues and tag them community responsibility.

If something is necessary but too complex, we should be asking it to be simplified instead of ignoring it.

(Also, after I started following your tweets, I realised that CRA is already helping its dependencies improve. That is great!)

@gaearon
Copy link
Contributor

gaearon commented Sep 3, 2016

I know that you don't work on things that you don't need (like Windows support)

This is not fair, Create React App supported Windows since its initial release, and we always test it.

You can analyse what makes SSR so complex, open necessary issues and tag them community responsibility.

So can you 😉 . We are not a giant team, there’s like four people working on React and they have a ton of other work to do that is currently a higher priority for us. We are inspired by the work @aickin and other people are doing to make SSR better in React, and we are keeping these efforts in mind as we work on React future.

@AlicanC
Copy link

AlicanC commented Sep 4, 2016

This is not fair, Create React App supported Windows since its initial release, and we always test it.

I didn't mean CRA :)

So can you 😉

Does that mean you would accept an issue here? I would get zero attention if I do it under my name. I also don't want to do this under other "solutions" (like React Server) because this complexity is the reason they exist. It will basically be saying "Let's fix these so people won't need your project." and you would probably get insulted and blocked for that on GitHub 😄

@comerc
Copy link

comerc commented Apr 4, 2017

louischan-oursky pushed a commit to louischan-oursky/create-react-app that referenced this pull request Jan 25, 2018
…piler so that you can import images and other files (references facebook#172)
@lock lock bot locked and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants