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

Support for server-side rendering #118

Merged
merged 8 commits into from Sep 8, 2016
Merged

Support for server-side rendering #118

merged 8 commits into from Sep 8, 2016

Conversation

wuct
Copy link
Contributor

@wuct wuct commented Sep 2, 2016

In order to use this middleware to develop a server-side rendering application, we need access to the stats. This PR is aimed to give the access to consumers.

How it works

app.use(webpackMiddleware(compiler, { serverSideRender: true })
app.use((req, res) => {
  const  assetsByChunkName = res.locals.webpackStats.toJson().assetsByChunkName
  // then use `assetsByChunkName` for server-sider rendering
})

If options.serverSideRender is true, the webpack-dev-middleware will set the stat object, which is generated by the latest build, to res.locals, then call next() to trigger the next middleware.

During the webpack building process, all requests, inclusive of non-files request, will be pending until the build is finished.

If `options.serverSideRender` is true, the middleware will set the `stat` object, which is generated by webpack, to `res`, and all requests will wait for building.
Support for options.serverSideRender
@ericclemmons
Copy link

I think the feature/idea is sound (I also do SSR), but wanted to ask a couple questions:

  • Setting the property on res seems odd, vs. something like res.locals or req.locals (I forget which) that are preferable for "custom" values.
  • I noticed that webpackDevMiddleware exports a fileSystem property on it. I had no idea about this until this PR, and didn't know if inspecting that object serves the same purpose?

Again, I think exposing stats in some form is useful, especially since you see several projects in the wild trying to accomplish something similar:

https://github.com/mjackson/web-starter/blob/master/modules/server/AssetsUtils.js

@wuct
Copy link
Contributor Author

wuct commented Sep 4, 2016

@ericclemmons I agree with setting the property on res is inappropriate. I believe we should use app.locals res.locals instead.

AFIK inspecting fileSystem means using fs.readdirSync() and fs.statSync() to get all info we need, which is sufficient but might not efficient.

EDITED: use res.locals because app is not accessible inside a middleware.

@@ -194,8 +197,16 @@ module.exports = function(compiler, options) {

// The middleware function
function webpackDevMiddleware(req, res, next) {
var goNext = function() {
if (!options.serverSideRender) return next()
Copy link
Member

Choose a reason for hiding this comment

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

Build fails because of this; it should be if(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@SpaceK33z
Copy link
Member

SpaceK33z commented Sep 4, 2016

Thanks, looks good now. @ericclemmons, you also agree?

EDIT: one more thing, could you document it in the README?

@wuct
Copy link
Contributor Author

wuct commented Sep 4, 2016

@SpaceK33z Sure, if this feature is good to be merged :)

@ericclemmons
Copy link

Looks good to me as well (with README update)!

@wuct
Copy link
Contributor Author

wuct commented Sep 8, 2016

Updated README
c8b7ba7

@SpaceK33z SpaceK33z merged commit 90af0d8 into webpack:master Sep 8, 2016
@SpaceK33z
Copy link
Member

Very nice work @wuct. Thank you.

@wuct
Copy link
Contributor Author

wuct commented Sep 8, 2016

@SpaceK33z You are welcome! Thanks for merging :)

@SpaceK33z
Copy link
Member

Released in 1.7.0.

wuct added a commit to wuct/webpack-dev-middleware that referenced this pull request Sep 12, 2016
…eware

* 'master' of https://github.com/webpack/webpack-dev-middleware:
  committed too quickly...
  Add not-functional test for server side render
  More tests
  fix typos
  Start with some tests!
  Fix inconsistencies with semi colons
  Use valid license for npm
  Document `reporter` option
  Add changelog for 1.7.0
  1.7.0
  Removing watching/watching.running check (webpack#100)
  Improve eslint linting and remove js-beautify
  Support for server-side rendering  (webpack#118)
@SpaceK33z SpaceK33z mentioned this pull request Sep 24, 2016
@richardscarrott
Copy link

richardscarrott commented Sep 27, 2016

Hi @wuct, I wondered how are you setting res.locals.webpackStats in production, i.e. when you're not using webpack-dev-middleware?

Also, when you say 'server-side rendering' do you mean rendering the app universally / isomorphically or just rendering an empty html page?

@wuct
Copy link
Contributor Author

wuct commented Sep 29, 2016

@richardscarrott Sorry for late replying. In production, we use a pre-built stat file which is generated by webpack-stats-plugin. Here is some simplified version of our code:

if (process.env.NODE_ENV !== 'production') {
  app.use(webpackMiddleware(compiler, { serverSideRender: true })
}

app.use((req, res) => {
  const { assetsByChunkName } = process.env.NODE_ENV !== 'production'
    ? res.locals.webpackStats.toJson()
    : require('./path/to/stat.json')

  // then use assetsByChunkName to render index.html
})

And yes, actually we are using React to rendering our app universally. To do this, we use webpack to create two bundles by providing two different webpack config files. One bundle is for client-side rendering, the other is for server-side rendering.

In the development mode, we not only use webpack-dev-middleware, but also use webpack-server-sider-render-middleware which would pass the bundle to res instead of passing stat.

The entire version of all server logic is:

if (process.env.NODE_ENV !== 'production') {
  app.use(webpackMiddleware(compiler, { serverSideRender: true })
  app.use(webpackSeverRenderMiddleware(serverCompiler))
}

app.use((req, res) => {
  const { assetsByChunkName } = process.env.NODE_ENV !== 'production'
    ? res.locals.webpackStats.toJson()
    : require('./path/to/stat.json')

  const renderer = process.env.NODE_ENV !== 'production'
    ? res.serverBundle
    : require('./path/to/prebuilt/serverBundle')

    // render all the stuff then response to client
  render(assetsByChunkName, req, res)
})

Hope this answer is helpful :)

@richardscarrott
Copy link

richardscarrott commented Sep 29, 2016

@wuct Thanks for the reply! Funnily enough I just put out something super similar to your webpack-server-render-middleware for hot server bundles webpack-hot-server-middleware, I like that approach a lot.

Really though, it's not this change in webpack-dev-middleware that is facilitating the server side rendering but actually it's your webpack-server-render-middleware... your middleware even adds the stats to the req so you don't really need this change in webpack-dev-middleware?

The main reason I raise this is I think the documentation added with this PR is a little misleading as it says 'Server-Side Rendering', as does the option name.

I wonder if it's worth updating the docs or even just delegate this functionality to another middleware, I think it could be done quite easily, something like this?

function webpackStatsMiddleware(compiler) {
    let latestStats;
    compiler.plugin('done', stats => latestStats = stats);
    return (req, res, next) => {
        req.locals.webpackStats = latestStats;
        next();
    }
}
app.use(webpackDevMiddleware(compiler));
app.use(webpackStatsMiddleware(compiler));

@wuct
Copy link
Contributor Author

wuct commented Sep 30, 2016

@richardscarrott I really like your approach that separates different functionality into different middleware. It is much simpler then mine. I agree with you current doc is confusing. I would like to revert this PR and maybe add links to webpack-hot-server-middleware and webpack-stat-middleware (after we make one) here.

@SpaceK33z how do you think?

@SpaceK33z
Copy link
Member

@richardscarrott, your example would not work completely. If the compiler is still compiling, and you refresh the page, this would mean you still get the old version. This can be very confusing. This is exactly what webpack-dev-middleware fixes. When making a request with the compiler still busy, it will delay the request till the compiler is done.

@wuct, we could remove it for the next major version, and maybe add a link to your package instead. However, I would like to have a clear upgrade path for people who might be using this PR.

@richardscarrott
Copy link

richardscarrott commented Sep 30, 2016

@SpaceK33z I'm not sure that's true by the virtue of the middleware stack as webpack-dev-middleware won't call next until the compiler is stable, that's how both webpack-hot-middleware and webpack-hot-server-middleware work alongside webpack-dev-midddleware.

I did notice that webpack-dev-middleware does on occasion call next early, e.g. if it's not a GET request or if the request doesn't satisfy the rules defined here but in practice I've not found I've had any subsequent middlewares that needed stats which didn't meet that criteria.

If that became a problem I think this would work:

function webpackStatsMiddleware(compiler, options = {}) {
    let latestStats;
    compiler.plugin('done', stats => latestStats = stats);
    return (req, res, next) => {
       options.waitUntilValid(() => {
           req.locals.webpackStats = latestStats;
           next();
        });
    }
}
const webpackDev = webpackDevMiddleware(compiler);
app.use(webpackDev);
app.use(webpackStatsMiddleware(compiler, {
    waitUntilValid: webpackDev.waitUntilValid
}));

Which would then allow you to mount it before webpack-dev-middleware if you really wanted to.

@SpaceK33z
Copy link
Member

@richardscarrott, ahhh you're right, I didn't realise you could use that trick. Ignore my previous comment ;).

@wuct
Copy link
Contributor Author

wuct commented Sep 30, 2016

Hmmm, I forgot that without options.serverSideRender, webpack-dev-middleware would let non-files requests pass directly. Thanks for reminding. Then, IMHO options.serverSideRender is still useful, maybe what we need is just a better name.

@SpaceK33z
Copy link
Member

Yeah it would be great if the name is a bit more generic. I'd accept a PR that changes the name to something sane (don't know a good one myself atm). Maybe also an idea to add a reference to @wuct's webpack-server-render-middleware package.

@richardscarrott
Copy link

richardscarrott commented Sep 30, 2016

@wuct / @SpaceK33z my personal opinion is that webpack-dev-middleware should be kept very focussed as it's the core of quite a few other packages but as I said before, my main concern really is the documentation and option name being misleading.

If the feature is going to stay then perhaps something along the lines of exposeStats would be a better name?

Or, I wonder if the functionality could be made more generic by perhaps providing an alwaysWaitUntilValid / blockAllRequests option...although thinking about it is there any reason it doesn't just always wait for the compiler, regardless of the request method / url, given it'll only ever be used in development?

@jhk2020
Copy link

jhk2020 commented Oct 25, 2016

Hi all,
Sorry if this sounds incredibly novice, but I'm a little confused as to when exactly the serverSideRender flag should be used. To ask another way, what limitations were there to the middleware before this pull request was merged in terms of server side rendering? The reason why I ask is because I'm having issues with building a universal app using webpack-dev-middleware where making any changes to my components and reloading the page results in an invalid checksum error. I'm wondering if my issue can be resolved by using this flag in the code or if the root of my problem is elsewhere. Adding to the confusion is the fact that a lot of example repos of universal apps (all created before the serverSideRender feature was merged) seem to use webpack-dev-middleware without any additional SSR-specific configuration. I'd really appreciate any input.

@coreyleelarson
Copy link

coreyleelarson commented Oct 26, 2016

@jhk2020 AFAIK, the only reason for the serverSideRender flag is to access the webpack stats from the in-memory file system, in order to dynamically include the bundled assets in your application. Your problem doesn't sound like it's related.

@clempat
Copy link

clempat commented Dec 5, 2016

@wuct if your code is splitted because of how it is written for the client side. How the requirement of the other chunk works as it is in-memory ? I would get module not found right.

@wuct
Copy link
Contributor Author

wuct commented Dec 7, 2016

@jhk2020 @coreyleelarson In addition to getting the stats, serverSideRender flag also delay all requests when the compiler is compiling. If we don't delay requests for pages, we will use the outdated stats (because the new stats is not ready) to build the page and the client will receive wrong links to asset files. This behavior is not easy to observe when the compiler is fast, but it's confusing when it occurs.

@wuct
Copy link
Contributor Author

wuct commented Dec 7, 2016

@clempat do you mind to share the relevant part of your webpack config?

@clempat
Copy link

clempat commented Dec 7, 2016

@wuct I will try to be the most specific possible.

I use webpack 2. I have two Webpack configuration, one for client and one for server. Each has a different entry point but share the same code. I use react and react-router. In react-router I use System.import for code spliting. Which would be interesting mostly for client. But this is part of the shared code.

Fyi I use commonjs2 as libraryTarget for server.

If I build and run the build all works fine on both side.

Problem

The problem come on dev server so with in-memory files. I use a similar configuration to yours presented there. If I was not spliting the code all would be fine. But as it is splitted. I get an error like module './1.server.bundle.js' not found. I guess that the first bundle cannot require further bundle as they don't rely on the in-memory files-system.

Possible solutions

I was thinking about few possibility.

  • Webpack ignore code splitting when target: node is used.
  • A way to find how I can share the file system
  • Condition on my code (ugly hack)

I hope I gave enough information to understand the problem I face. And I am thankful to you for any input you could bring...

@richardscarrott
Copy link

@clempat As you have discovered when targeting 'node' Webpack will use the native 'require' to pull in chunks so it only supports the standard file system. You can however disable code splitting via config using the LimitChunkCountPlugin:

{
    plugins: [
        ...
        new webpack.optimize.LimitChunkCountPlugin({
            maxChunks: 1
        })
    ]
}

Having done this you still need to access the server entry which can only be done by reading it in from the memory fs and loading the module from buffer or string, which is how webpack-hot-server-middleware works.

@clempat
Copy link

clempat commented Dec 7, 2016

@richardscarrott Thanks a lot. It works perfectly !!! Perfect !! I will just need to check why i have checksum was invalid

@clempat
Copy link

clempat commented Dec 7, 2016

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

Successfully merging this pull request may close these issues.

None yet

7 participants