404 with await ctx.render('404') has application/json content type

[niftylettuce]

this is because the body is empty per

koa/lib/response.js

Line 135 in 971115c

if (null == val) {

so the only way to fix is to not only set ctx.throw(404) but to also reset the content type when you're sending a 404 error

we should maybe account for 404 errors here before we unset everything?

[jonathanong]

what is ctx.render()?

[niftylettuce]

for koa@next when I have error middleware like this at the top of my file, I have to explicitly say ctx.type = 'html'; for example... (maybe I'm doing it wrong?) -- this was from code I did the other day

// initialize try/catch error handling right away
// adapted from: https://github.com/koajs/onerror/blob/master/index.js
// https://github.com/koajs/examples/issues/20#issuecomment-31568401
app.use(async (ctx, next) => {

  try {
    await next();
    if (ctx.status === 404)
      ctx.throw(404);
  } catch (err) {

    if (err.name) {
      switch (err.name) {
      case 'ValidationError':
        err.status = 400;
        if (err.errors) {
          let errors = _.map(
            Object.values(err.errors),
            (e) => {
              switch (e.kind) {
              case 'required':
                return `${_.upperFirst(e.path)} is required`;
              default:
                return _.trimEnd(e.message, '.');
              }
            }
          ).join(', ');
          err.message = `${err.message}: ${errors}`;
        }
        break;
      }
    }

    ctx.status = err.status || 500;
    ctx.body = Boom.create(err.status, err.message).output.payload;

    app.emit('error', err, ctx);

    let type = ctx.accepts(['text', 'json', 'html']);

    if (!type) {
      ctx.status = 406;
      ctx.body = Boom.notAcceptable().output.payload;
    }

    switch (type) {
    case 'html':
      if (ctx.status === 404) {
        ctx.type = 'html';
        // https://github.com/koajs/koa/issues/646
        await ctx.render('404');
      } else {
        ctx.flash('error', err.message);
        ctx.redirect('/');
      }
      break;
    case 'json':
      ctx.body = JSON.stringify(ctx.body);
      break;
    default: // case 'text':
      ctx.type = 'text';
      ctx.body = JSON.stringify(ctx.body, null, 2);
      break;
    }

  }

});

[niftylettuce]

see how i use ctx.render in that block -- it's my nunjucks rendering via https://github.com/hanai/koa-nunjucks-promise

[niftylettuce]

also for reference, Boom is https://github.com/hapijs/boom

[jonathanong]

yeah i always throw the error and let upstream error handlers handle those pages. i also don't set headers unless i know the status code isn't gonna be an error, which makes things a little more robust, but is more annoying to code.

i also don't do content negotiation anymore :P hard to cache

[niftylettuce]

how the heck do we do this then? for example: https://github.com/niftylettuce/ratelimit/blob/master/src/index.js#L67-L74

[jonathanong]

you'd have to remove the headers yourself. :(

[niftylettuce]

how do I do that? @jonathanong very confused here...

[jonathanong]

koa/lib/context.js

Line 121 in fcacb35

this.res._headers = {};

hacks galore!

unless you want to make a PR for #571 :)

[niftylettuce]

@jonathanong how do we override that onerror, via app.onerror = fn? is there any examples of someone doing this?

[jonathanong]

just doing app.context.onerror = should work, but onerror should only be used when extremely necessary.

instead, this and similar middleware should be relying on an upstream error handler like:

app.use(function* (next) {
  try {
    yield next
  } catch (err) {
    // handle error
    this.status = err.status || 500
    this.body = { message: err.message }
  }
})

but then you have to do special cases for middleware like that rate limit one... which is why it's better just to have a public API

[jonathanong]

oh shit. i read the code wrong. i thought you were setting that body after await next but you're early returning.

what's the issue?

[niftylettuce]

the issue is that headers are removed from the request on an error, when I don't want them to be

[jonathanong]

only if onerror handles it. if you make your own middleware, you can do wahtever you want.

[PlasmaPower]

Does c826467 resolve this?

[jonathanong]

yes c826467 should resolve this