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

Enables to toggle server side rendering when running the server. #833

Closed

Conversation

hackhat
Copy link

@hackhat hackhat commented Sep 5, 2016

Configuration has a new key named serverSideRendering that can be toggled between true and false. Related to: #642, #634

…iguration has a new key named `serverSideRendering` that can be toggled between `true` and `false`. Related to: kriasoft#642, kriasoft#634
@frenzzy
Copy link
Member

frenzzy commented Sep 6, 2016

It may be better to just not use server.js if you want to disable the SSR?

@hackhat
Copy link
Author

hackhat commented Sep 6, 2016

The issue with that is that you need extra work to setup, and managing an extra index.html page. Probably could be rendered once and then served. Then you would need another server config to serve this index.html page including all the static files + hot reload...
I think this would help to get started for now, what do you think?

@frenzzy
Copy link
Member

frenzzy commented Sep 7, 2016

SSR in RSK already configured and allows to do following:

  • The code written in one place will work on the client and on the server.
  • DEV: the client-side and server-side hot reload just works.
  • PROD: website content is visible to search engines like Google, the page loads faster.
  • PROD: slow react SSR can be resolved by caching or by compiling server responses to static files during build process.

So, I do not understand your benefits.
If you still consider that you do not need SSR just look towards RSB.

@sauravskumar
Copy link

sauravskumar commented Sep 8, 2016

@frenzzy i think this can be great pull request... since server side rendering as we all know produces some delay... (since ajax has to be performed and wait for rendering to happen after that)... if this pull request can be modified such to perform client side rendering in case of real user and ssr for google crawl bot then it would provide the user with a better user experience in terms of lower page load times (since the app does not need to perform ajax and rendering on server side)... and it would also be good for seo since ssr can be performed in case of google bot... I see many major companies taking this approach now a days such as flipkart...

Great pull request... though the value of serverSideRendering should be decided based on user agent for that functionality...
@hackhat
+1

@hackhat
Copy link
Author

hackhat commented Sep 9, 2016

@frenzzy This is entirely for development reasons; Sometimes you get server side errors and the whole app doesn't start or behaves unexpectedly; If is just client side rendering, you can debug errors easily; At least that is what I've noticed.

@hackhat
Copy link
Author

hackhat commented Sep 24, 2016

Another reason, that happened just now, is that sometimes there is an error and the server doesn't even start, in this case I will disable server side rendering and investigate the issue on the client side.

Copy link
Collaborator

@langpavel langpavel left a comment

Choose a reason for hiding this comment

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

I cannot see benefits of this, but pointing you to better directions

@@ -11,6 +11,7 @@

export const port = process.env.PORT || 3000;
export const host = process.env.WEBSITE_HOSTNAME || `localhost:${port}`;
export const serverSideRendering = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use export const disableServerSideRendering = !process.env.DISABLE_SERVER_SIDE_RENDERING;

@@ -27,7 +27,7 @@ import models from './data/models';
import schema from './data/schema';
import routes from './routes';
import assets from './assets'; // eslint-disable-line import/no-unresolved
import { port, auth } from './config';
import { port, auth, serverSideRendering } from './config';
Copy link
Collaborator

Choose a reason for hiding this comment

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

disableServerSideRendering

@@ -84,31 +84,30 @@ app.use('/graphql', expressGraphQL(req => ({
// -----------------------------------------------------------------------------
app.get('*', async (req, res, next) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Disable SSR on router level, not inside handler, add default app loader

@hackhat
Copy link
Author

hackhat commented Oct 17, 2016

Thanks @langpavel but I'm not sure if is worth investing more time fixing the conflicts and adding your suggestions if it won't be accepted anyway.

@langpavel
Copy link
Collaborator

Anyway thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants