koajs / examples

Example Koa apps
4.52k stars 744 forks source link

404 - no correct 404 page for exceptions in downstream middleware #83

Closed i-erokhin closed 7 years ago

i-erokhin commented 8 years ago

In the example, custom 404 works only if no response from any downstream middleware.

Нowever, if any downstream middleware throws an exception, pageNotFound() middleware don't works (default 404 page will be sent).

See my pull request with a test (currently broken): https://github.com/koajs/examples/pull/82/files

hemanth commented 8 years ago

Even with exception and ok being seprate middlewares?

i-erokhin commented 8 years ago

ok() middleware and 'when GET /ok' is just an additional test for ensure what normal pages works as expected.

i-erokhin commented 8 years ago

Obviously, code after yield next will not work if exception throws in next().

app.use(function *pageNotFound(next){
  yield next;

  if (404 != this.status) return;

  // we need to explicitly set 404 here
  // so that koa doesn't assign 200 on body=
  this.status = 404;

  switch (this.accepts('html', 'json')) {
    case 'html':
      this.type = 'html';
      this.body = '<p>Page Not Found</p>';
      break;
    case 'json':
      this.body = {
        message: 'Page Not Found'
      };
      break;
    default:
      this.type = 'text';
      this.body = 'Page Not Found';
  }
})
hemanth commented 8 years ago

explicitly set 404 here

^ SGTM

i-erokhin commented 8 years ago

This is not my code, this is the code from the example: https://github.com/koajs/examples/blob/master/404/app.js , and it not works as expected. Or do you think that the code is correct?

bananaappletw commented 7 years ago

Sorry to say. But, I think you are not correct. Usually, 404 page is place at the end of middleware. When no route is matched, it show the 404 page. Or you don't specify the error page, the koa will deal it automatically. Default, koa set status code as 404. https://github.com/koajs/koa/blob/master/lib/application.js#L133 In this line https://github.com/koajs/koa/blob/master/lib/application.js#L219 using statuse to get message get message()

bananaappletw commented 7 years ago

And, actually your code is not corresponded with what you say. Your code doesn't throw exception. In usual, if you throw exception. Your http status code should be 5xx. https://en.wikipedia.org/wiki/List_of_HTTP_status_codes#5xx_Server_Error

hemanth commented 7 years ago

Thank you.