117 handle resource 404s

[joanniclaborde]

This PR handles broken resources and preprocessors.

Resources

An error will be rendered instead of the requested page if a resource:

• Cannot be retrieved because of a connection error, or the returned JSON is invalid. Returned status code: 500
• Is valid but has a >=300 status code (https://github.com/solidusjs/solidus-client/pull/3 can also change the status code). Returned status code: the resource's status code

Note: if a resource has the optional=true option set, then any error described above will be ignored, and the page will render as if the resource never existed.

Preprocessors

If a preprocessor throws an exception, a 500 error page will be rendered. Instead of throwing an exception, the preprocessors can now also:

• Return the status code to render instead of the current page: return 404;
• Return a url to redirect to (with status code 302): return '/home';
• Return an array containing the status code and url to redirect to: return [301, '/home'];

Rendering errors

When an error occurs, Solidus will first try to find a corresponding error template to use (views/404.hbs, views/500.hbs, etc.). If the template cannot be found, the standard status code message will be rendered (404 Not Found, 500 Internal Server Error, etc.).

When in dev mode, or when a .json page is requested, a JSON object will be rendered instead of the error page described above. For example:

{
  "status": 500,
  "error": "Error running preprocessor 'signup_page.js'",
  "message": [
    "TypeError: Cannot read property 'b' of undefined",
    "    at module.exports (/vagrant/mysite/preprocessors/signup_page.js:5:10)",
    "    at module.exports (/vagrant/solidus/lib/preprocessor_worker.js:13:15)",
    "    at handle (/vagrant/solidus/node_modules/worker-farm/lib/child/index.js:37:8)",
    "    at process.<anonymous> (/vagrant/solidus/node_modules/worker-farm/lib/child/index.js:43:3)",
    "    at process.EventEmitter.emit (events.js:98:17)",
    "    at handleMessage (child_process.js:318:10)",
    "    at Pipe.channel.onread (child_process.js:345:11)"
  ],
  "context": {
    "url": {
      "host": "...",
      ...

Fixes #117 Fixes #128

Note: this branch is based on #116

[joanniclaborde]

Warning: rebased over #116, delete your local branch!

[joanniclaborde]

Warning: rebased over #116, delete your local branch!

[localjo]

Tried this out on jasonaldean.com/master, everything works smoothly as described above.

I think the rendering of error templates in non---dev mode needs a way to define a default error template for cases where there is a non-200 code but no template for that specific status code exists.

[joanniclaborde]

Warning: rebased over #116, delete your local branch!

[joanniclaborde]

Yes that's a good idea, what do you think @pushred. If we do that, we should remove the support for the numeric error pages (500.hbs) and let that master error template handle specific errors itself. Just like we let the dev setup helpers himself.

And we need to find a good error template name, something that won't clash with existing pages. solidus_error.hbs?

[pushred]

The error pages have access to the context in the dev mode JSON right? If so yeah a single template is totally sufficient. Why not just provide that context with .json on the end in dev mode like other views?

I'm fine with views/solidus_error.hbs for now.

[pushred]

@josiahsprague Please try this out on youngmoney.com as well, with a production deploy if all looks well locally. Would like to see how things go over the remainder of the week.

[localjo]

I tried this out on youngmoney.com, everything looks good, so I'll push it over to production after a thumbs up. One thing I noticed is that there isn't any kind of page context available on an error page. That might be useful, especially when solidus_error.hbs is added, to display information to the user about the error.

[pushred]

I think the idea is just that you should be able to reproduce the issues in dev. And we also send that data to Sentry when logging the exception. Course that only works if you use Sentry, but it's really our recommendation for exception monitoring.

[localjo]

Well the friendly error pages (404.hbs, 500.hbs, solidus_error.hbs) wouldn't apply in --dev mode. I think they're just to provide some useful error to a visitor that lands on a dead page. Having some information about the error (whether the page doesn't exist, or if it's just a temporary error and they should try later, etc) is probably useful there.

[pushred]

Oh gotcha, perhaps to be Handlebars-friendly it'd be good to parameterize the text status with a boolean value to handle custom messaging:

{{#if not_found }}
    Not found, sorry!
{{/if}}

Handlebars 3 finally adds if/else chains, so the final {{else}} would be a catch-all. We could bundle this PR with a Handlebars upgrade for Solidus 2.0..

[pushred]

Actually that would significantly delay rolling this out to many sites. This would work too, now:

{{#if client_error }}
    <!-- 4xx -->
    {{ status }} ({{ code }})
{{/if}}

{{#if server_error }}
    <!-- 5xx -->
    {{ status }} ({{ code }})
{{/if}}

Both would be nice though.

[localjo]

I was just thinking something like

<p>Ooops! {{ error_message }}</p>
<small>Details: {{ status_code }}</small>

No need to distinguish between client/server errors. UX can design friendlier pages as long as we give them access to info about the error in the page context.

[joanniclaborde]

OK so I'll treat the error page like any other page: it will be rendered with the usual context, but I'll also add an error key:

{
  "url": {...},
  "page": {...},
  "resources": {...},
  ...
  "error": {
    "status": 404,
    "error": "Error retrieving resource 'post'",
    "message": {
      "status": "error",
      "messages": [
        "invalid_status"
      ],
      "response": "[{\"code\":\"json_post_invalid_id\",\"message\":\"Invalid post ID.\"}]"
    }
  }
}

I'll also stop logging 404 messages as errors, so they won't appear in the live log. Sounds good?

[localjo]

Yeah, that's exactly what I was thinking. :)

[joanniclaborde]

solidus_error.hbs sounds stupid, let's just use error.hbs, I don't see why such a page would exist on a site yet.

[joanniclaborde]

Another suggestion: how about returning a 500 instead of a 404 when a STATIC resource is not found? So we get a notification about it.

[localjo]

Yeah, I think a 500 error would be appropriate in that case. Since it is an error on the server, not the client. Maybe a 502?

The server, while acting as a gateway or proxy, received an invalid response from the upstream server it accessed in attempting to fulfill the request.

[pushred]

:+1: