dispatchAsync eats uncaught errors

[vesparny]

I have a component, which is a children of another component that is listening on some stores tied to async operations.

If I try to throw an error in this sub-component, the error is not caught because it happens while dispatching:
https://github.com/acdlite/flummox/blob/master/src/Flux.js#L119

If I sorround the dispatching code with try catch, I'm able to log the error

body => {
    try{   
        this._dispatch({
          actionId,
          body,
          async: 'success'
        });
    }catch(e){
       console.log(e);
    }
   },

I'd like to be able to re-throw the exception here, because it's useful especially while developing.
The problem is that we are in a Promise context, so the error is lost.

For instance if you throw new Error() here:
https://github.com/vesparny/flux-immutable-example/blob/master/src/components/FrameworkDetail.js#L18
You'll see that the error is silent, and in the console you can see that only the begin handler is dispatched, because the success one throws an error.

[acdlite]

Ah, good catch!* This should be an easy fix.

*totally unintended pun, I swear

[vesparny]

:)

[acdlite]

Hmm, well, this works:

return promise
  .then(
    body => {
      setTimeout(() =>
        this._dispatch({
          actionId,
          body,
          async: 'success'
        })
      , 0);
    },
    error => {
      setTimeout(() =>
        this._dispatch({
          actionId,
          error,
          async: 'failure',
        })
      , 0);

      return Promise.reject(error);
    }
  );

but it feels pretty hacky. And it means if you await an async action, the action hasn't dispatched yet. Not good.

The other option is to use a catch handler

return promise
  .then(
    body => {
      this._dispatch({
        actionId,
        body,
        async: 'success'
      });
    },
    error => {
      this._dispatch({
        actionId,
        error,
        async: 'failure',
      });

      return Promise.reject(error);
    }
  )
  .catch(error => setTimeout(() => { throw error }, 0));

but that also feels hacky because 1) promise-based operations aren't supposed to throw errors, and 2) the error is being thrown during a different turn of the event loop from when it actually occurred.

I think a better way to handle this would be to have Flux emit an error event, the same way it emits a dispatch event. How does that sound?

[acdlite]

Released as part of v2.12.0. If you want to "un-swallow" the errors, you can do something like this:

flux.addListener('error', error => setTimeout(() => { throw error }, 0));

I would only do this during development, though, if at all.

[vesparny]

Hey man, sorry about the late response here but I've been busy.

I saw you fix and it works.

But..
Even though I totally agree with you when you say that a promise is not supposed to throw, the reality is that it potentially could.
Since you are calling something external like the Dispatcher#dispatch, I think the best approach would be to surround that call with a try/catch block.
This way we can catch the exception and rethrow it in the next tick.
Doing this seems to be the normal behaviour, I mean that if an error occurs the right thing to do is not hiding it, but make the system aware of it, in a natural way (rethrow). Then the developer will be in charge of handling it.

McFly does something similar.
https://github.com/kenwheeler/mcfly/blob/9a45f3e8095d40685a09594256148d58bc3a2143/src/Action.js#L6

Hiding the error and make it catchable only subscribing to the error event on the flux instance feels a bit weird to me, especially if I don't know that I can do it just because I'm too lazy to read the docs :)

This is just my 2 cents, your solution work pretty well by the way.
Handling errors in promises is always a mess :)

[acdlite]

even though [...] a promise is not supposed to throw, the reality is that it potentially could.

Not unless you do it intentionally using setTimeout(). All errors that occur inside a .then() callback are (by design) gobbled up and turned into a rejected promise. It's part of the spec.

I mean that if an error occurs the right thing to do is not hiding it, but make the system aware of it, in a natural way (rethrow)

That's the natural way to handle errors in synchronous code, because you can use try-catch. But try-catch doesn't work for errors that occur in separate ticks of the event loop, hence the common Node-style pattern of passing an error object as the first argument to a callback. (Even using ES7 async functions, which do allow you to use try-catch through some wonderful generator and promise magic, if an unhandled error occurs, the function returns a rejected promise.)

With promises, the natural way to handle errors is to reject. I agree it's kind of weird, and probably the most contentious part about promises, but (since error-throwing is a synchronous operation) it makes sense if you think of promises as occurring in a totally separate time dimension from its calling context.

In practical terms, the reason "rethrowing" an error using setTimeout() is bad is because there's no way to catch it after that, because it happens during the next tick. Which is obviously bad.

McFly does something similar.

Tell me, how would you catch the error on line 11 after it's thrown? :)

I think the best approach would be to surround that call with a try/catch block.

Wrapping in a try-catch block has the same effect as the second code snippet from my comment above.

All of this is not to say that I'm not open to figuring out a better way to handle errors in Flummox. I just don't think rethrowing is the right solution. For now, I would recommend using the error event so you can make your own decisions.

[vesparny]

Tell me, how would you catch the error on line 11 after it's thrown? :)

I guess the only way do to so is to usewindow.onerror handler.

The one we are discussing about is probably the most controversial Promise part :)

Anyways, the goal of the issue was to have the ability to notice errors, this way we are now able to log somewhere errors deriving from a developer syntax error for instance. And this is good 👍