shellscape/koa-webpack

Continuation error when using lazy mode

dpatti opened this issue · 2 comments

  • Node Version: v7.9.0
  • NPM Version: yarn v0.22.0
  • koa Version: v2.2.0
  • koa-wepback Version: v0.4.0

Example snippet:

app.use(async (ctx, next) => {
  console.log("Handing control to middleware");
  await next();
  console.log("Received control from middleware");
  console.log("Body is set?", !!ctx.body);
});
app.use(koaWebpack({
  compiler: webpack(webpackConfig),
  dev: {
    publicPath: '/',
    noInfo: true,
    // lazy: true is important
    lazy: true,
  },
}));

Expected Behavior

The koa-webpack middleware blocks on and handles requests for resources that webpack would handle.

Actual Behavior

Control is passed back to koa, and later the middleware tries to send a response even though the server has already sent a 404 (or potentially something else). Here is the console output of the above:

Handing control to middleware
webpack building...
Received control from middleware
Body is set? false
webpack built f7bc127c6bdda045c586 in 920ms
_http_outgoing.js:371
    throw new Error('Can\'t set headers after they are sent.');
    ^

Error: Can't set headers after they are sent.

How can we reproduce the behavior?

Using the snippet above. More specifically, the issue exists in this code:

return async (context, next) => {
  await waitMiddleware();
  await dev(context.req, {
    end: (content) => {
      context.body = content;
    },
    setHeader: context.set.bind(context),
    locals: context.state
  }, next);
};

The call to dev, which is the function provided by webpack-dev-middleware,
does not explicitly return anything, though it does, under some branches, call
return next(). Since the next we are passing above is a function that
returns a promise, this works some of the time. However, with lazy: true, it
will return undefined, and the await above will end synchronously, ending the
middleware and passing control back to koa, which will probably return a 404.
However, the webpack-dev-middleware may eventually compile the file and then
try to set the headers and body to send the response, even though koa has
already finished with it.

I would propose a solution like this:

return async (context, next) => {
  await waitMiddleware();
  await new Promise((resolve, reject) => {
    dev(context.req, {
      end: (content) => {
        context.body = content;
        resolve();
      },
      setHeader: context.set.bind(context),
      locals: context.state
    }, () => resolve(next()));
  });
};

I'm also not sure if this affects the hot middleware, though it seems to be
using a similar pattern of passing the next to the callback function. If this
fix looks acceptable, I am willing to open a pull request with the changes.

Also, I believe this is the cause of #46 as well. The only difference is that next is called late when koa-middleware realizes it doesn't want to handle the request, and then the static middleware tries to set the body within a promise when it has already been set.

That looks like a good fix to me. We desperately need some simple tests around this to make sure that one commit to the next doesn't break other things. Basic functionality. Is that something you'd be willing to add with the PR containing your fix?