hono icon indicating copy to clipboard operation
hono copied to clipboard

SSG: If the status code is anything other than 200, we should raise an error by default

Open usualoma opened this issue 1 year ago • 9 comments

What is the feature you are proposing?

Even now, afterResponseHook can be used to check status codes and process errors, but by default, processing continues with any status code other than 200. In general, if a request for SSG returns anything other than 200, an error should be reported. (Ignoring it and continuing is also not a good idea.)

usualoma avatar Feb 09 '24 10:02 usualoma

@usualoma For reference, the current specification is intentionally generating status codes other than 200 as described. The discussion took place here: https://github.com/honojs/hono/pull/2044

watany-dev avatar Feb 09 '24 10:02 watany-dev

@watany-dev Thanks! It was helpful.

But true, for example, but although I checked your PR, I think this change should have been merged. Because this is a "404 that failed after a GET request to a route that only supports POST", right? This looks to me like a "misconfiguration that is about to be output and should be reported as an error".

https://github.com/honojs/hono/pull/2044/files#diff-44ab9aa69e76a7b36fc0fe4a12eb85ca0def556924a8e5088110ec2040f84b6fL142

Here's what @yusukebe is saying,

https://github.com/honojs/hono/pull/2044#issuecomment-1901930361

For example, it would be nice if toSSG() could also output /404.html by writing the following.

app.get('/404', (c) => {
  if (!isSSGContext(c)) {
    c.status(404)
  }
  return c.render(<h1>Not found! </h1>)
})

For example, an error should be reported rather than silently output when

app.get(
  '/shops/:id',
  ssgParams([
    { id: 1, name: 'Shop 1' },
    { id: 2, name: 'Shop 2' }, // something wrong for this shop. return 404 or 500 from this route
    { id: 3, name: 'Shop 3' },
  ]),
  async (c) => {
    const shop = await getShop(c.req.param('id'))
    if (!shop) {
      return c.notFound()
    }
    return c.render(
      <div>
        <h1>{shop.name}</h1>
      </div>
    )
  }
)

If we were to do it a little more cleverly, we might do the following, but that would likely confuse the user.

  • Output even a 404 when static routing.
  • No 404s for dynamic routing

mmm, after all, isn't it strange that the following remain as expected values?

https://github.com/honojs/hono/pull/2044/files#diff-44ab9aa69e76a7b36fc0fe4a12eb85ca0def556924a8e5088110ec2040f84b6fL142

usualoma avatar Feb 09 '24 11:02 usualoma

When it comes to POST requests, POST is generally a request to change state, and even if the POST results is generated by render(), there is often POST data in it. I do not think it is correct behavior for POST to be written out by default.

usualoma avatar Feb 09 '24 11:02 usualoma

Or... perhaps, as far as CloudFlare Pages are concerned, what we need is this.

app.get('/404', onlySSG(), (c) => {
  return c.render(<h1>Not found! </h1>)
})

usualoma avatar Feb 09 '24 12:02 usualoma

Also, if 404s due to accidents are not ignored, it will be quite painful to generate a sitemap, since it will be necessary to select the target files when generating the sitemap.

#2125

usualoma avatar Feb 09 '24 12:02 usualoma

Oh, or maybe it's a bit redundant, but maybe 404.html should be generated from the plugin.

As this comment states,

https://github.com/honojs/hono/issues/2125#issuecomment-1920167497

Write the following.

import { notFound } from 'hono/ssg/plugins'

toSSG(app, fs, {
  plugins: [notFound()], // write /404.html by default
})
export default app

or

import { notFound } from 'hono/ssg/plugins'

toSSG(app, fs, {
  plugins: [notFound({routes => ['/404.html', '/blog/404.html']})], // write /404.html and /blog/404.html
})
export default app

usualoma avatar Feb 10 '24 11:02 usualoma

I'll write simply one more time

For example, when we call an external API to generate content, we sometimes get a "temporary error that results in a non-200 result". Currently, in that case, I think the result (contains an error message) is saved without issuing a warning. (Am I wrong?) When you publish it to the public server, the error page will serve with status code 200.

I think this behavior should be fixed.

usualoma avatar Feb 19 '24 09:02 usualoma

Note: The following is an example of the use of the This is a bit of a long shot, so we'll address it around mid-March when we have more time.

watany-dev avatar Feb 19 '24 09:02 watany-dev

Hi @watany-dev

Thanks for the comment. Sorry, I'm not trying to rush you. I just thought we needed to decide how to do this by hono.

I think we need to get @yusukebe , who wrote https://github.com/honojs/hono/pull/2044#issuecomment-1901930361, to comment. This, too, is not a rush, though.

usualoma avatar Feb 19 '24 09:02 usualoma