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

Export render function #1292

Closed

Conversation

pugnascotia
Copy link
Contributor

In order to facilitate server-side rendering (#1100), change the production webpack config to package the bundle as a UMD module, making it usable in a number of environments. The module's name is derived from the application's name in package.json.

For this to be meaningful, the bundle's entrypoint must export a function, so I've added a basic render function to index.js. The existing browser mount must be wrapped in a guard condition to stop it failing in a server environment.

@kpuputti
Copy link

kpuputti commented Jan 4, 2017

I tested this approach and it works nicely!

However, a server rendering setup will most likely not render the whole application with no props, but integrate with the application router with the URL given to the server. Would this require CRA to bundle e.g. React Router and this render function somehow integrate with it, or how could this problem be solved?

@kpuputti
Copy link

kpuputti commented Jan 4, 2017

Sorry, after reading the linked issue, I realised that the whole setup can be done in the application! So this approach would work nicely with any routing setup.

@kpuputti
Copy link

kpuputti commented Jan 4, 2017

Would this break chunk loading since the default Webpack target is "web"?

@pugnascotia
Copy link
Contributor Author

Chunk loading behaviour would remain unchanged, though you'd still need a strategy to cope with instances of e.g. require.ensure on the server. For example:

https://github.com/ryanflorence/example-react-router-server-rendering-lazy-routes

@pugnascotia
Copy link
Contributor Author

Any feedback on this?

@pugnascotia
Copy link
Contributor Author

@gaearon Any chance of this getting merged?

@alex-at-oclc
Copy link

I like the approach of supporting both client and server from the same build. By sharing the Webpack config it introduces a few compromises - for example, you can't access the runtime process.env on the server. Nothing that can't be worked-around though.

I'd prefer to add src/server.js as a second (optional) entry point in the Webpack config, leaving the existing src/index.js unaltered and clean for those uninterested in SSR. For most SSR apps, the exported render function is going to need the request and to send the complete response. For example, react-router needs the path from the request, and may respond with non-200 status codes. Helmet needs to write to the document head. Redux needs to write the initial state somewhere.

I have a similar setup working, also with SSR and hot-module-reloading in the dev server - albeit currently with a separate server compile. I got some ideas from #172, but managed to get it working all in a single WebpackDevServer in the same process. I added a serverSideRender flag to package.json - without this set, the CRA behaviour is unaltered and still beginner-friendly. Setting it to true enables SSR in the dev server and the build. One other thing I do is to pickup the asset manifest and pass that into the render function, so that it knows where to look for the bundle files. There's no coupling to anything other than node-fetch on the server. Most of the changes are to scripts/start.js.

Is it worth me cleaning that up and submitting a PR? I'm unclear whether there is a fighting chance of getting something like this accepted.

@pugnascotia
Copy link
Contributor Author

My impression (and I hope @gaearon will correct me) is that CRA is not intended to be an all singing, all dancing boilerplate - there are others more aligned with that goal. The reason that the UMD bundle change is more reasonable is because it's relatively non-instrusive and leaves the heavy lifting of SSR to the developer. All the coding around routing and redux state is do-able with the UMD bundle as proposed - I know because I have a branch of my own boilerplate that uses it, and does both routing and state. But hey, that's just my opinion.

@gaearon
Copy link
Contributor

gaearon commented Feb 4, 2017

Hey, sorry for no reply. I intend to review this, but as you can see by other PRs, there’s quite a queue to review so it will take a while.

I’m very busy with work on React itself right now so I will ask collaborators to look at PRs. New features are likely to be reviewed later than bugfixes.

We’ll come back to this, but not right now. Thanks!

@pugnascotia
Copy link
Contributor Author

OK, thanks for the update.

@pugnascotia
Copy link
Contributor Author

I've rebased off master and tidied up a couple things, no biggies.

@alex-at-oclc
Copy link

@pugnascotia: I agree with your comments. I've not gone that much further than you - all of the routing, state management etc. is still left to the application code. The key difference is that I have the dev server automatically calling the render function (if serverSideRender is true) - which requires a standard function signature. I think it's valuable to run SSR in dev mode, as it helps early identification of broken SSR or when SSR renders different content to the browser.

That aside, your PR delivers a lot of value with very little impact.

@gaearon gaearon added this to the 0.10.0 milestone Feb 24, 2017
@pugnascotia pugnascotia force-pushed the export-render-function branch 3 times, most recently from ada13a6 to 79a7e0a Compare March 22, 2017 14:30
Derive a library name from the project name in package.json.

In the index.js template, don't render the App unless window is defined,
and export a render-to-string function.
@pugnascotia
Copy link
Contributor Author

Hello maintainers,

I've rebased the branch on master again, and written a basic end-to-end test script, mostly by copying the existing 'simple' script.

I've also updated the documentation to cover the changes. Feedback welcome!

@Timer
Copy link
Contributor

Timer commented Mar 22, 2017

I believe not having to eject is crucial here when using code splitting because we're going to be pushing it hard in 0.10 via webpack performance hints.
Can we exclude export default function render() { return renderToString(<App />); } by default and detect when it's added, which auto generates the related files for server side rendering (including the second webpack bundle)?

@pugnascotia
Copy link
Contributor Author

I'm up for changing that, can we thrash what we need? How about:

  1. Detect either an ES6 default export or a named export of render from the bundle.
  2. If present, make the bundle a UMD module (to allow non-Node, non-code-splitting users to still do SSR)
  3. Generate a second webpack config, which must:
    3.1 Set the target to node
    3.2 Use a different output folder - not sure what to call this, how about server-build? Or the current contents of build could be moved to build/client and the server bundle could go to build/server. Looking for input here.
    3.3 Other changes will likely be required. For example, the server build should probably consider only JavaScript resources - everything else should handled exactly as-is by the current production build.

Exclusions:

  1. CommonsJS exports are ignored
  2. We won't provide anything to actually execute the server render.

Start implementing intelligent server bundling by parsing the app's
entrypoint and looking for an exported function.
@Timer Timer mentioned this pull request May 5, 2017
32 tasks
@gaearon
Copy link
Contributor

gaearon commented May 8, 2017

I’m growing hesitant about this, though I really appreciate the effort that went into it.
Mostly because the landscape has changed since our original discussion.

There are a few things that concern me:

  • Reliance on parsing as a hint (IMO it’s a bit too implicit even by our standards).
  • Dynamic import() in Webpack 2 doesn't have a very good story for server side rendering (AFAIK).
  • Mostly, I’m concerned that we’ll ship a simple-but-not-good integration, and our users will miss out on all the great things that Next.js does.

In the meantime, Next.js has grown more popular, keeps getting very non-trivial optimizations, and has integrations that are familiar to our users.

I think that, at this point, it might be better to promote Next.js more heavily, and just embrace that this is the problem space they understand better.

What do you think?

@Timer
Copy link
Contributor

Timer commented May 8, 2017

I'm a little conflicted. I believe we can do some great things in this space, and I don't think parsing out an exported render is too implicit.

IMO Next.js is very opinionated (for good reasons), but I think generic support in CRA would be appreciated by many.
Dynamic import() works afaik, but simply doesn't resolve synchronously which means SSR will show "loading" components. I believe this is a fair trade off when people can use react-async-component or the newer react-loadable. We could even hint this when using SSR.

I think it'd be fair to at least hold off until 0.11 instead of closing it so we have longer to evaluate the pros & cons.

As for the choice of espree, would it not make more sense to use babylon since we already depend on babel? This would make sure our modules don't grow and we don't have divergence of language features in the future.

@gaearon
Copy link
Contributor

gaearon commented May 8, 2017

I think it'd be fair to at least hold off until 0.11 instead of closing it so we have longer to evaluate the pros & cons.

Sounds good.

As for the choice of espree, would it not make more sense to use babylon since we already depend on babel?

Is there a downside to a separate file like src/server.js?

@Timer
Copy link
Contributor

Timer commented May 8, 2017

Is there a downside to a separate file like src/server.js?

There's nothing wrong with this other than requiring some (possibly) duplicated code; it's probably a bit more explicit (which is better).

@Timer Timer modified the milestones: 0.11.0, 0.10.0 May 8, 2017
@pugnascotia
Copy link
Contributor Author

So would the idea with src/server.js be to use it as a separate Webpack entry point, and generate the server bundle that way?

For dynamic imports, I've been experimenting with defining an extra CRA Babel package that pulls in babel-plugin-dynamic-import-node. This just turns dynamic imports into a regular requires that satisfies a promise. That should make it possible to write server-side code that can render a complete page. Thoughts?

@Timer
Copy link
Contributor

Timer commented May 9, 2017

So would the idea with src/server.js be to use it as a separate Webpack entry point, and generate the server bundle that way?

Yup, that's the idea! This removes some of that implicit parsing magic (I'm really sorry you spent the time working on it, but we might use it in the future).

For dynamic imports, I've been experimenting with defining an extra CRA Babel package that pulls in babel-plugin-dynamic-import-node. This just turns dynamic imports into a regular requires that satisfies a promise. That should make it possible to write server-side code that can render a complete page. Thoughts?

I'm not very experienced with SSR so maybe you can help shed some light on this.
Do we want to use webpack for server side rendering? From my understanding, we wouldn't want to webpack code that's going to run server side -- wouldn't you just let node parse out the modules and dependencies?
In that event, we'd just run it against babel with plugins like babel-plugin-dynamic-import-node and re-enable modules as cjs.


Out of interest in your time @pugnascotia, please don't move forward with any changes yet (unless you really want to). I don't want you to waste your time until we figure out the specifics (in case we change our minds). 😄

@pugnascotia
Copy link
Contributor Author

It's a fair point, but Node still doesn't support ES6 modules yet, so some kind of compilation would still required. Same if you're using experimental ES features, or minifying for performance reasons, etc.

@pugnascotia
Copy link
Contributor Author

(BTW, my original reason for getting involved in this was SSR using Nashorn in a Java / Spring app. So, you know, node-what? :-))

@Timer
Copy link
Contributor

Timer commented May 9, 2017

We'd just run it against our react-app babel preset which enables all stage-4 plugins and a few experimentals (and most stage-3).

@cr101
Copy link
Contributor

cr101 commented May 16, 2017

I'm a React beginner and I believe that building a React app with server-side rendering is essential simply because it just makes sense.
I really hope that CRA will support SSR out of the box like Next.js does.

@raymondsze
Copy link

@pugnascotia
I have question that if this UMD approach could work in development mode (i.e webpack dev server) as the files are served in-memory.

@pugnascotia
Copy link
Contributor Author

My original intent with this PR was to make it possible to consume a CRA app as a library / module, so that I could performing server rendering in a Java application using the Nashorn JS engine. @gaearon makes a good point that other projects are tackling SSR as a primary concern - given that at the moment there's no coherent story in CRA around SSR, and that we could add value right now in a discrete way by exporting the app as a UMD module, what do we think about adding the export and coming back to SSR later?

@lock lock bot locked and limited conversation to collaborators Jan 20, 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