Improper exception handling?

[robink]

I believe there is a problem in the current way exception are handled on the server rendering phase:

072f4c5#diff-0655f086119d60b6cf62f877f4128fa8R77

If the navigateAction fails somehow, you're still rendering the app in an inconsistant state.

Eg:

1. start rendering app
2. NAVIGATE_START is dispatched
3. Page is currently rendering, then crash some reason, for example because one translation is missing
4. An exception is thrown, catched here.
5. App renders with a loader (because we are still in NAVIGATE_START phase) and nothing will ever happens

[gpbl]

Hi, thanks for reporting!
The case you are referring to should never happen. Could you reproduce it?

Server-side, there's no NAVIGATE_START to be handled since the app has not been rendered yet: as both the promises have been fulfilled (i.e. the navigateAction did finish as well), the route will be rendered always when NAVIGATE_SUCCESS has been dispatched:

Promise.all([
      context.executeAction(loadIntlMessages, { locale: req.locale }), // lets ignore this 
      context.executeAction(navigateAction, { url: req.url })
    ]).then(() => renderApp(req, res, context, next))
      .catch(() => renderApp(req, res, context, next));

When executing navigateAction:

1. NAVIGATE_START is dispatched
2. (optionally, route actions will be called)
3. NAVIGATE_SUCCESS is dispatched
4. Render the app to static markup (in renderApp)
5. If an error occurs while rendering, then pass it to the next middleware

The trick here is that renderApp does catch a rendering error thanks to a try/catch clause.

The catch function is there because if actions would return an error, we still want to render the app.

What do you think?

[robink]

Well... If meta.loadingTitle translation is missing, then you can reproduce it.
https://github.com/gpbl/isomorphic500/blob/master/src/stores/HtmlHeadStore.js#L70

I guess this one is the only one that should not be missing....

[gpbl]

Oh yes I see it now. The case of the HtmlHeadStore is weird. Not sure how to deal with it. Let me think :)

[gpbl]

So since the errors fired from route actions should always have a status code, we could write this:

Promise.all([
      context.executeAction(loadIntlMessages, { locale: req.locale }),
      context.executeAction(navigateAction, { url: req.url })
    ]).then(() => renderApp(req, res, context, next))
      .catch((err) => {
        if (!err.statusCode || !err.status) {
          next(err);
        }
        else {
          renderApp(req, res, context, next);
        }
      });

[robink]

It seems perfectly handled this way :) nice!

[gpbl]

🎉