New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: default error handling of Epics #94

Open
jayphelps opened this Issue Aug 6, 2016 · 54 comments

Comments

@jayphelps
Member

jayphelps commented Aug 6, 2016

The middleware is intentionally designed to accept a single, root Epic. This means that all your Epics are typically composed together using combineEpics(...epics).

The problem with this approach is that if an exception is thrown inside one of your epics and it isn't caught by you, it will "bubble" up all the way to the root epic and cause the entire output stream to terminate aka error(). As an example, Observable.ajax() currently throws exceptions for non-200 responses. So one bad AJAX request can terminate all of your epics and your app no longer listens for actions to handle side effects.

You can demo this sort of behavior here: http://jsbin.com/yowiduh/edit?js,output notice that it works the first time, errors the second, and any subsiquent time does nothing because the epic has terminated!

This is not unlike most programming models (where an uncaught exception terminates the "process"). In fact, this behavior is the same as if you were doing the equivalent in imperative JS with generators or similar. So RxJS is "correct" in the implementation.

However, because this has critical implications, we've been discussing adding some default behavior to mitigate this; especially for people who may be fairly new to Rx and not realize the implications.

Here are a couple options:

  1. .catch() it and and resubscribe to the root epic, basically "restarting" epics to continue listening.
    • Epics that maintain internal state will have lost that state completely, even ones that didn't cause the exception.
  2. or have combineEpics() listen for individual epics throwing exceptions and restart only the one that produced the error.

Alternative suggestions are welcome.


If we proof-of-concept option 1. it might look something like this:

http://jsbin.com/kalite/edit?js,output

const output$ = rootEpic(action$, store).catch((error, source) => {
  Observable.throw(error, Scheduler.async).subscribe();
  return source;
});

output$.subscribe(store.dispatch);

I'm not sure we want to do Observable.throw(error, Scheduler.async).subscribe(); but I used it here because I do want the exception to be thrown so window.onerror sees it and it shows up in the console as expected, but we also need to return the original source observable so I'm using Scheduler.async to "defer" the exception. There's prolly a better way to do this. A simple setTimeout prolly is ok instead.

Cc/ @blesh

@jayphelps jayphelps added this to the v1.0.0 milestone Aug 6, 2016

@jayphelps jayphelps changed the title from RFC: default to middleware catching errors and continuing with original stream to RFC: default error handling of Epics Aug 6, 2016

@jayphelps jayphelps added the RFC label Aug 6, 2016

@jayphelps

This comment has been minimized.

Show comment
Hide comment
@jayphelps

jayphelps Sep 28, 2016

Member

I've decided on do option 1 for now, feedback welcome. PR to come shortly.

Member

jayphelps commented Sep 28, 2016

I've decided on do option 1 for now, feedback welcome. PR to come shortly.

@jayphelps

This comment has been minimized.

Show comment
Hide comment
@jayphelps

jayphelps Oct 1, 2016

Member

Turns out I was wrong saying that option 1 would allow unrelated epics to maintain their internal state. Not sure why I thought that in hindsight (confirmation bias). Effectively option 1 is in fact still letting the entire root epic terminate and then "restarting" it entirely with a new subscription.

I've updated the original post above to reflect that, as well as the the jsbin to reflect this--the console will show an incrementing number which will reset back to zero whenever the other ajax epic errors.

Again, that means option 1, catching errors at the root epic level, would cause all epics to lose their internal state, and restart.

Option 2 is catching them individually inside combineEpics:

export const combineEpics = (...epics) => (...args) =>
  merge(
    ...epics.map(epic =>
      epic(...args)
        // When epic emits error, rethrow it async and resubscribe.
        // Otherwise, errors will cause cascading termination of all epics. 
        ::_catch((error, source) => {
          setTimeout(() => { throw error; }, 0);
          return source;
        })
    )
  );

This however has its own caveats.

  • If you don't use combineEpics, obviously it won't catch them--we could also catch the root epic as well, so that seems like a doable solution?
  • Having combineEpics do this feels like magic that will most often go unknown to people and confuse them why this behavior doesn't happen when they invoke epics manually when doing higher-order epics aka composition.
const epic1 = (action$, store) => stuff();
const epic2 = (action$, store) => epic1(moreStuff(), store);

We could later mitigate this by introducing a paved way to "fork" epics, as we've been experimenting with for a while. Forking is just like composition except you don't need to pass along the store cause it'll be passed along for you. We could catch errors in forks as well.

  • Lastly, this would break some people who are right now catching their own errors and logging them. We've actually helped several people do this, and it would be particularly onerous because if they wanted to continue they'd have to effectively re-create combineEpic that _doesn't have this catching behavior.
const rootEpic = (action$, store) =>
  combineEpics(epic1, epic2)(action$, store)
    .catch((e, source) => {
      logTheErrorToAPIOrSomething(e);
      return source;
    });

Summary

I feel kinda back to the drawing board on this one. Perhaps we should just add some clear documentation around the existing behavior and call it good for now until/if we come up with a real solution?

Member

jayphelps commented Oct 1, 2016

Turns out I was wrong saying that option 1 would allow unrelated epics to maintain their internal state. Not sure why I thought that in hindsight (confirmation bias). Effectively option 1 is in fact still letting the entire root epic terminate and then "restarting" it entirely with a new subscription.

I've updated the original post above to reflect that, as well as the the jsbin to reflect this--the console will show an incrementing number which will reset back to zero whenever the other ajax epic errors.

Again, that means option 1, catching errors at the root epic level, would cause all epics to lose their internal state, and restart.

Option 2 is catching them individually inside combineEpics:

export const combineEpics = (...epics) => (...args) =>
  merge(
    ...epics.map(epic =>
      epic(...args)
        // When epic emits error, rethrow it async and resubscribe.
        // Otherwise, errors will cause cascading termination of all epics. 
        ::_catch((error, source) => {
          setTimeout(() => { throw error; }, 0);
          return source;
        })
    )
  );

This however has its own caveats.

  • If you don't use combineEpics, obviously it won't catch them--we could also catch the root epic as well, so that seems like a doable solution?
  • Having combineEpics do this feels like magic that will most often go unknown to people and confuse them why this behavior doesn't happen when they invoke epics manually when doing higher-order epics aka composition.
const epic1 = (action$, store) => stuff();
const epic2 = (action$, store) => epic1(moreStuff(), store);

We could later mitigate this by introducing a paved way to "fork" epics, as we've been experimenting with for a while. Forking is just like composition except you don't need to pass along the store cause it'll be passed along for you. We could catch errors in forks as well.

  • Lastly, this would break some people who are right now catching their own errors and logging them. We've actually helped several people do this, and it would be particularly onerous because if they wanted to continue they'd have to effectively re-create combineEpic that _doesn't have this catching behavior.
const rootEpic = (action$, store) =>
  combineEpics(epic1, epic2)(action$, store)
    .catch((e, source) => {
      logTheErrorToAPIOrSomething(e);
      return source;
    });

Summary

I feel kinda back to the drawing board on this one. Perhaps we should just add some clear documentation around the existing behavior and call it good for now until/if we come up with a real solution?

@slorber

This comment has been minimized.

Show comment
Hide comment
@slorber

slorber Oct 24, 2016

Related problems are experienced by redux-saga users regularly.

I think the solution is to implement "supervision strategy" that actor systems have. Sagas are generally implemented on the backend by using actor systems like Akka (even Greg Young, best advocate of eventsourcing/DDD said Akka is a saga framework).

The idea of the actor systems resilience is that the system should be able to heal itself on its own with supervision strategy. If one part of the actor tree fails, we can declare which strategy to adopt for that part of the tree.

The epic restarting itself is actually one of the most used supervision strategy in actor systems.

I think the behavior should rather not be encoded directly inside combineEpics but rather as a wrapper for each epic, because one should be able to setup fine-grained behavior per epic

More infos on this issue: redux-saga/redux-saga#570

slorber commented Oct 24, 2016

Related problems are experienced by redux-saga users regularly.

I think the solution is to implement "supervision strategy" that actor systems have. Sagas are generally implemented on the backend by using actor systems like Akka (even Greg Young, best advocate of eventsourcing/DDD said Akka is a saga framework).

The idea of the actor systems resilience is that the system should be able to heal itself on its own with supervision strategy. If one part of the actor tree fails, we can declare which strategy to adopt for that part of the tree.

The epic restarting itself is actually one of the most used supervision strategy in actor systems.

I think the behavior should rather not be encoded directly inside combineEpics but rather as a wrapper for each epic, because one should be able to setup fine-grained behavior per epic

More infos on this issue: redux-saga/redux-saga#570

@jayphelps

This comment has been minimized.

Show comment
Hide comment
@jayphelps

jayphelps Oct 25, 2016

Member

@slorber people can opt into automatic retry already today with the .retry() operator:

const somethingEpic = action$
  action$.ofType(SOMETHING)
    .mergeMap(action => Observable.throw('BAD'))
    .retry();

Edit: this swallows the error though! That's probably bad.
Instead, you can do this:

const somethingEpic = action$
  action$.ofType(SOMETHING)
    .mergeMap(action => Observable.throw('BAD'))
    .catch((e, source) => {
      // rethrow error so it shows up in the console
      setTimeout(() => { throw e; }, 0);
      // source is the chain above the catch, so this "retries"
      return source;
    });

You could make this a custom operator, of course.

Perhaps the answer to this whole problem is just to make this operator + usage more obvious in the docs...but I dunno.

This whole effort was mostly because I felt people would practically always want an epic to restart if it errors out. The argument against I guess is that those errors are uncaught exceptions for a reason--we can't guarantee the behavior of the app continues correctly if we always restart them. However, in JS world I would argue people are very much used to apps which may have errors but try to chug along anyway, even if some things are completely broken. In the redux-observable case, if a single epic errors out without being caught, all of their epics will terminate. This cascade is pretty dubious and more analogous to traditional apps with a "crash"--but the user won't receive any notification it happened (unless the dev adds one).

Member

jayphelps commented Oct 25, 2016

@slorber people can opt into automatic retry already today with the .retry() operator:

const somethingEpic = action$
  action$.ofType(SOMETHING)
    .mergeMap(action => Observable.throw('BAD'))
    .retry();

Edit: this swallows the error though! That's probably bad.
Instead, you can do this:

const somethingEpic = action$
  action$.ofType(SOMETHING)
    .mergeMap(action => Observable.throw('BAD'))
    .catch((e, source) => {
      // rethrow error so it shows up in the console
      setTimeout(() => { throw e; }, 0);
      // source is the chain above the catch, so this "retries"
      return source;
    });

You could make this a custom operator, of course.

Perhaps the answer to this whole problem is just to make this operator + usage more obvious in the docs...but I dunno.

This whole effort was mostly because I felt people would practically always want an epic to restart if it errors out. The argument against I guess is that those errors are uncaught exceptions for a reason--we can't guarantee the behavior of the app continues correctly if we always restart them. However, in JS world I would argue people are very much used to apps which may have errors but try to chug along anyway, even if some things are completely broken. In the redux-observable case, if a single epic errors out without being caught, all of their epics will terminate. This cascade is pretty dubious and more analogous to traditional apps with a "crash"--but the user won't receive any notification it happened (unless the dev adds one).

@slorber

This comment has been minimized.

Show comment
Hide comment
@slorber

slorber Oct 25, 2016

This is the exact same problem with redux-saga: people have a root saga and fork child sagas, but an error in a single child saga does kill the root saga, and thus the other child sagas, (so there's no saga left in the end). This happened to me because of bad connections: a single http request failure could make my whole app unusable. There's however a way to know it crashed as we can receive uncaught errors in a callback and display something to the user.

Still think it should be fine-grained, as sometimes you want the error to propagate to the parent (which may retry or propagate also), and sometimes you want it to retry. Generally, at the very top of the tree, you want to retry as the sagas/epics are generally unrelated and work in isolation, but inside an isolated saga, you may want to propagate to the parent so that it's the parent that does the retry.

About RxJS retry(), according to the marble, will it resubscribe to past events as well? Because a Saga should conceptually not receive twice the same events. In backend systems, sagas/process managers generally use at-least-once semantics to be sure to receive the async messages, but make sure to handle it only once in any case by keeping reference/version/whatever of already handled messages. Not sure to understand if retry() is really suitable to the problem because the marble seems to resubscribe to the whole stream. Not doing RxJS for a long time so maybe it's just a problem of type of source observable (subject/hot/cold) ?

slorber commented Oct 25, 2016

This is the exact same problem with redux-saga: people have a root saga and fork child sagas, but an error in a single child saga does kill the root saga, and thus the other child sagas, (so there's no saga left in the end). This happened to me because of bad connections: a single http request failure could make my whole app unusable. There's however a way to know it crashed as we can receive uncaught errors in a callback and display something to the user.

Still think it should be fine-grained, as sometimes you want the error to propagate to the parent (which may retry or propagate also), and sometimes you want it to retry. Generally, at the very top of the tree, you want to retry as the sagas/epics are generally unrelated and work in isolation, but inside an isolated saga, you may want to propagate to the parent so that it's the parent that does the retry.

About RxJS retry(), according to the marble, will it resubscribe to past events as well? Because a Saga should conceptually not receive twice the same events. In backend systems, sagas/process managers generally use at-least-once semantics to be sure to receive the async messages, but make sure to handle it only once in any case by keeping reference/version/whatever of already handled messages. Not sure to understand if retry() is really suitable to the problem because the marble seems to resubscribe to the whole stream. Not doing RxJS for a long time so maybe it's just a problem of type of source observable (subject/hot/cold) ?

@jayphelps

This comment has been minimized.

Show comment
Hide comment
@jayphelps

jayphelps Oct 25, 2016

Member

@slorber the marble is displaying the behavior with the assumption that the source is lazy/cold. So resubscribing causes it to start producing as-new. Most Observables are this way, but Subjects are hot/multicast. (you mention these terms, so you prolly already understand 😄 )

The ActionsObservable we provide epics is a little confusing to think about deeply in that itself is lazy/cold but it subscribes internally to a Subject (which contains the real stream of actions internally) which is hot. Don't get hung up on this though. The main take away is that for any practical reason, resubscribing to action$ will not replay any previous actions. So using .retry() will, on error, resubscribe to any future actions from that point on. In the case of redux-observable epics using it on the top-level, the term "retry" is a bit of misnomer and instead can be thought of as "ignore the error and restart the epic"

Member

jayphelps commented Oct 25, 2016

@slorber the marble is displaying the behavior with the assumption that the source is lazy/cold. So resubscribing causes it to start producing as-new. Most Observables are this way, but Subjects are hot/multicast. (you mention these terms, so you prolly already understand 😄 )

The ActionsObservable we provide epics is a little confusing to think about deeply in that itself is lazy/cold but it subscribes internally to a Subject (which contains the real stream of actions internally) which is hot. Don't get hung up on this though. The main take away is that for any practical reason, resubscribing to action$ will not replay any previous actions. So using .retry() will, on error, resubscribe to any future actions from that point on. In the case of redux-observable epics using it on the top-level, the term "retry" is a bit of misnomer and instead can be thought of as "ignore the error and restart the epic"

@slorber

This comment has been minimized.

Show comment
Hide comment
@slorber

slorber Oct 25, 2016

I see. I guess you keep a hot action stream for event replay / hot code reloading, and make epic stream cold on purpose to avoid receiving past events on failure. Makes sense to me.

So yes I think this pattern should rather be documented instead of a default retry strategy in combineEpics which would probably be too magical and not flexible in case you don't want this behavior

slorber commented Oct 25, 2016

I see. I guess you keep a hot action stream for event replay / hot code reloading, and make epic stream cold on purpose to avoid receiving past events on failure. Makes sense to me.

So yes I think this pattern should rather be documented instead of a default retry strategy in combineEpics which would probably be too magical and not flexible in case you don't want this behavior

@jayphelps

This comment has been minimized.

Show comment
Hide comment
@jayphelps

jayphelps Oct 25, 2016

Member

@slorber

I guess you keep a hot action stream for event replay / hot code reloading, and make epic stream cold on purpose to avoid receiving past events on failure. Makes sense to me.

Not sure I follow? To clarify: if you use .retry() on the top-level stream returned in an epic, it will not replay previous actions. On error, it will simply "restart" the epic, listening for future actions.

The real stream of actions that redux-observable maintains internally is hot, which is one of the reasons why previous actions are not replayed--by design.

Member

jayphelps commented Oct 25, 2016

@slorber

I guess you keep a hot action stream for event replay / hot code reloading, and make epic stream cold on purpose to avoid receiving past events on failure. Makes sense to me.

Not sure I follow? To clarify: if you use .retry() on the top-level stream returned in an epic, it will not replay previous actions. On error, it will simply "restart" the epic, listening for future actions.

The real stream of actions that redux-observable maintains internally is hot, which is one of the reasons why previous actions are not replayed--by design.

@benlesh

This comment has been minimized.

Show comment
Hide comment
@benlesh

benlesh Oct 25, 2016

Collaborator

Are we looking for a way to sandbox individual epics? I think that happens anyhow.

If we restart or otherwise "handle" unhandled errors by default

Benefits

  1. Probably the most ergonomic for the most users.
  2. Developers don't have to learn proper error handling with observables.

Risks

  1. Sometimes users will want their epic to die in the event of an error, or errors. Think of repeated ajax errors or something, perhaps they want it to just die after 100 attempts.
  2. Blanket catching errors that are unknown to developers might squelch feedback they're otherwise expecting.
  3. It's harder to "opt-out" of automagic error handling
  4. Perhaps harder to control strategy. immediate retry? incremental step back?
  5. Developers don't have to learn proper error handling with observables.

If we don't handle any errors

Benefits

  1. Developers can configure their error handling in whatever way they choose.
  2. Developers forced to learn error handling techniques in Rx.

Risks

  1. Might be less ergonomic for the general use case where most developers do not want there epics to die after an error.
  2. Developers forced to learn error handling techniques in Rx.
Collaborator

benlesh commented Oct 25, 2016

Are we looking for a way to sandbox individual epics? I think that happens anyhow.

If we restart or otherwise "handle" unhandled errors by default

Benefits

  1. Probably the most ergonomic for the most users.
  2. Developers don't have to learn proper error handling with observables.

Risks

  1. Sometimes users will want their epic to die in the event of an error, or errors. Think of repeated ajax errors or something, perhaps they want it to just die after 100 attempts.
  2. Blanket catching errors that are unknown to developers might squelch feedback they're otherwise expecting.
  3. It's harder to "opt-out" of automagic error handling
  4. Perhaps harder to control strategy. immediate retry? incremental step back?
  5. Developers don't have to learn proper error handling with observables.

If we don't handle any errors

Benefits

  1. Developers can configure their error handling in whatever way they choose.
  2. Developers forced to learn error handling techniques in Rx.

Risks

  1. Might be less ergonomic for the general use case where most developers do not want there epics to die after an error.
  2. Developers forced to learn error handling techniques in Rx.
@jayphelps

This comment has been minimized.

Show comment
Hide comment
@jayphelps

jayphelps Oct 26, 2016

Member

just jotting down some rough "create epic helper" ideas to provide some default behavior (including restarting on error), with escape hatches via optional config arg.

// having options first seems the most visually appealing,
// but that means an awkward overload if they don't provide options
const somethingEpic = createEpic({ restartOnError: false }, action$ =>
  action$.ofType(PING)
    .map(() => pong())
);

// so if default behavior is fine, first arg is epic implementation
// itself. but again..overloads are awkward
const somethingEpic = createEpic(action$ =>
  action$.ofType(PING)
    .map(() => pong())
);

// putting options last means we don't need an
// overload, but it sure looks awkward for arrow fns
// without a block
const somethingEpic = createEpic(action$ =>
  action$.ofType(PING)
    .map(() => pong()),
  { restartOnError: false } 
);
Member

jayphelps commented Oct 26, 2016

just jotting down some rough "create epic helper" ideas to provide some default behavior (including restarting on error), with escape hatches via optional config arg.

// having options first seems the most visually appealing,
// but that means an awkward overload if they don't provide options
const somethingEpic = createEpic({ restartOnError: false }, action$ =>
  action$.ofType(PING)
    .map(() => pong())
);

// so if default behavior is fine, first arg is epic implementation
// itself. but again..overloads are awkward
const somethingEpic = createEpic(action$ =>
  action$.ofType(PING)
    .map(() => pong())
);

// putting options last means we don't need an
// overload, but it sure looks awkward for arrow fns
// without a block
const somethingEpic = createEpic(action$ =>
  action$.ofType(PING)
    .map(() => pong()),
  { restartOnError: false } 
);
@rgbkrk

This comment has been minimized.

Show comment
Hide comment
@rgbkrk

rgbkrk Oct 26, 2016

Member

I'm definitely in favor of redux-observable not handling any errors for the reasons Ben mentioned:

  1. Developers can configure their error handling in whatever way they choose.
  2. Developers forced to learn error handling techniques in Rx.

I'd rather see recipes/patterns for error handling, exponential backoff, and retrying.

Member

rgbkrk commented Oct 26, 2016

I'm definitely in favor of redux-observable not handling any errors for the reasons Ben mentioned:

  1. Developers can configure their error handling in whatever way they choose.
  2. Developers forced to learn error handling techniques in Rx.

I'd rather see recipes/patterns for error handling, exponential backoff, and retrying.

@slorber

This comment has been minimized.

Show comment
Hide comment
@slorber

slorber Oct 26, 2016

same for me. It's also the path taken by @yelouafi on redux-saga btw

slorber commented Oct 26, 2016

same for me. It's also the path taken by @yelouafi on redux-saga btw

@jayphelps

This comment has been minimized.

Show comment
Hide comment
@jayphelps

jayphelps Oct 26, 2016

Member

If we go that route, I'm struggling to decide how the docs should prescribe when to use .retry() and when not to? For many, it seems like they'd want to add it to the end of every one of their epics..which seems pretty annoying. Thoughts? Seems silly that a single unhandled ajax call error can cause the whole app to come crashing down, though it could have kept chugging along in many cases.

Member

jayphelps commented Oct 26, 2016

If we go that route, I'm struggling to decide how the docs should prescribe when to use .retry() and when not to? For many, it seems like they'd want to add it to the end of every one of their epics..which seems pretty annoying. Thoughts? Seems silly that a single unhandled ajax call error can cause the whole app to come crashing down, though it could have kept chugging along in many cases.

@rgbkrk

This comment has been minimized.

Show comment
Hide comment
@rgbkrk

rgbkrk Oct 26, 2016

Member

@harph sung high praise for the examples/navigation section. I think more in-context examples would be good.

I feel bad that all the examples I can provide are so far in the weeds of a domain area that they would not be helpful to general users (unless they're hacking on our code).

Member

rgbkrk commented Oct 26, 2016

@harph sung high praise for the examples/navigation section. I think more in-context examples would be good.

I feel bad that all the examples I can provide are so far in the weeds of a domain area that they would not be helpful to general users (unless they're hacking on our code).

@BerkeleyTrue

This comment has been minimized.

Show comment
Hide comment
@BerkeleyTrue

BerkeleyTrue Oct 26, 2016

Collaborator
  1. Developers can configure their error handling in whatever way they choose.
  2. Developers forced to learn error handling techniques in Rx.

@jayphelps I'm also in favor of this

Collaborator

BerkeleyTrue commented Oct 26, 2016

  1. Developers can configure their error handling in whatever way they choose.
  2. Developers forced to learn error handling techniques in Rx.

@jayphelps I'm also in favor of this

@benlesh

This comment has been minimized.

Show comment
Hide comment
@benlesh

benlesh Oct 26, 2016

Collaborator

I think maybe RxJS can provide an interesting operator or two around this problem to help. Requires more thought though.

Collaborator

benlesh commented Oct 26, 2016

I think maybe RxJS can provide an interesting operator or two around this problem to help. Requires more thought though.

@salanki

This comment has been minimized.

Show comment
Hide comment
@salanki

salanki Nov 29, 2016

As a data point, this is how we do error handling in our (react native) app. A custom combineEpics that adds a repeat() to all epics.

return output$.repeat();

The point of the repeat() is to allow error catching in the epic with catch(x => Observable.of(errorActionCreator(x))). This pattern can then be used on both epics (hot observables) and cold ones that would just end up in an endless loop if adopting the catch((x, stream) => Observable.merge(Observable.of(errorActionCreator(x)), stream)) pattern everywhere.

On our top level combined epic we add a catch-all just as described earlier in this thread. This catch-all is only to be used as a last resort. All epics are expected to take care of their own errors.

It would be interesting to know what patterns people have adopted, especially for turning errors into actions.

salanki commented Nov 29, 2016

As a data point, this is how we do error handling in our (react native) app. A custom combineEpics that adds a repeat() to all epics.

return output$.repeat();

The point of the repeat() is to allow error catching in the epic with catch(x => Observable.of(errorActionCreator(x))). This pattern can then be used on both epics (hot observables) and cold ones that would just end up in an endless loop if adopting the catch((x, stream) => Observable.merge(Observable.of(errorActionCreator(x)), stream)) pattern everywhere.

On our top level combined epic we add a catch-all just as described earlier in this thread. This catch-all is only to be used as a last resort. All epics are expected to take care of their own errors.

It would be interesting to know what patterns people have adopted, especially for turning errors into actions.

@jayphelps

This comment has been minimized.

Show comment
Hide comment
@jayphelps

jayphelps Nov 29, 2016

Member

@salanki I believe using repeat() in that way would prevent your epics from shutting down gracefully in a server-render, but most people don't do SSR anyway. Just something to note.

Member

jayphelps commented Nov 29, 2016

@salanki I believe using repeat() in that way would prevent your epics from shutting down gracefully in a server-render, but most people don't do SSR anyway. Just something to note.

@meticoeus

This comment has been minimized.

Show comment
Hide comment
@meticoeus

meticoeus Jan 22, 2017

I'm also in favor of redux-observable not handling errors, but I think it would be really helpful if it would log uncaught exceptions so you can find out which epic is letting errors slip through.

I've also found that it silently crashes when exceptions are thrown in reducers, making some simple syntax errors hard to track down.

meticoeus commented Jan 22, 2017

I'm also in favor of redux-observable not handling errors, but I think it would be really helpful if it would log uncaught exceptions so you can find out which epic is letting errors slip through.

I've also found that it silently crashes when exceptions are thrown in reducers, making some simple syntax errors hard to track down.

@jayphelps

This comment has been minimized.

Show comment
Hide comment
@jayphelps

jayphelps Jan 22, 2017

Member

@meticoeus right now no errors should be swallowed by redux-observable. Errors in your reducers should also not cause any errors or other interactions with your epics. If this is happening and you're confident it's redux-observable, could you create an issue with a way to reproduce? Definitely want to fix 😄

You can see that exceptions bubble all the way here: http://jsbin.com/konebug/edit?js,output

Member

jayphelps commented Jan 22, 2017

@meticoeus right now no errors should be swallowed by redux-observable. Errors in your reducers should also not cause any errors or other interactions with your epics. If this is happening and you're confident it's redux-observable, could you create an issue with a way to reproduce? Definitely want to fix 😄

You can see that exceptions bubble all the way here: http://jsbin.com/konebug/edit?js,output

@jtomaszewski

This comment has been minimized.

Show comment
Hide comment
@jtomaszewski

jtomaszewski Feb 3, 2017

Here's some ready-to-use code based on what the others said above:

// lib/helpers/epic.js
export const wrapEpic = epic => (...args) =>
  epic(...args).catch((error, source) => {
    setTimeout(() => { throw error }, 0)
    return source
  })

// modules/auth/epic.js
import { combineEpics } from "redux-observable"
import { wrapEpic } from "lib/helpers/epic"
export const epic = combineEpics( ...[
  epic1,
  epic2,
].map(wrapEpic) )

// app/rootEpic.js
import { combineEpics } from "redux-observable"
import { wrapEpic } from "lib/helpers/epic"
import authEpic from "modules/auth/epic"
export default combineEpics( ...[
  authEpic,
].map(wrapEpic) )

I added the wrapEpic to every place I use combineEpics and now all the epics handle the errors properly. I think it's necessary - if I added it only to the rootEpic, then some of the epics weren't restarted properly.

EDIT Though I've got some problems in HMR: errors that were thrown before are being thrown again after hot reload, even though the new epic is properly replaced and the old epics shouldn't be working anymore (?). Any1 has a guess how to fix it?

P.S. If you still want your epic to stop and never restart, then instead of throwing an error, there are also other ways do it (f.e. https://github.com/Reactive-Extensions/RxJS/blob/master/doc/api/core/operators/never.md ) .

jtomaszewski commented Feb 3, 2017

Here's some ready-to-use code based on what the others said above:

// lib/helpers/epic.js
export const wrapEpic = epic => (...args) =>
  epic(...args).catch((error, source) => {
    setTimeout(() => { throw error }, 0)
    return source
  })

// modules/auth/epic.js
import { combineEpics } from "redux-observable"
import { wrapEpic } from "lib/helpers/epic"
export const epic = combineEpics( ...[
  epic1,
  epic2,
].map(wrapEpic) )

// app/rootEpic.js
import { combineEpics } from "redux-observable"
import { wrapEpic } from "lib/helpers/epic"
import authEpic from "modules/auth/epic"
export default combineEpics( ...[
  authEpic,
].map(wrapEpic) )

I added the wrapEpic to every place I use combineEpics and now all the epics handle the errors properly. I think it's necessary - if I added it only to the rootEpic, then some of the epics weren't restarted properly.

EDIT Though I've got some problems in HMR: errors that were thrown before are being thrown again after hot reload, even though the new epic is properly replaced and the old epics shouldn't be working anymore (?). Any1 has a guess how to fix it?

P.S. If you still want your epic to stop and never restart, then instead of throwing an error, there are also other ways do it (f.e. https://github.com/Reactive-Extensions/RxJS/blob/master/doc/api/core/operators/never.md ) .

@mmcgahan

This comment has been minimized.

Show comment
Hide comment
@mmcgahan

mmcgahan Mar 1, 2017

@jayphelps I think I'm having the same issue that @meticoeus is having with redux-observable appearing to swallow errors and crash the root epic.

I've slightly tweaked the JSBin you shared to put the error in the reducer

http://jsbin.com/rosiyapali/edit?js,console

The error gets thrown and hits window so it's definitely not being swallowed, but I also don't know how to catch it - it doesn't call onError in the pingEpic so I can't retry/restart. I feel like I must be missing something obvious, but it might be a case where redux-observable doesn't provide the right hook to handle the error

mmcgahan commented Mar 1, 2017

@jayphelps I think I'm having the same issue that @meticoeus is having with redux-observable appearing to swallow errors and crash the root epic.

I've slightly tweaked the JSBin you shared to put the error in the reducer

http://jsbin.com/rosiyapali/edit?js,console

The error gets thrown and hits window so it's definitely not being swallowed, but I also don't know how to catch it - it doesn't call onError in the pingEpic so I can't retry/restart. I feel like I must be missing something obvious, but it might be a case where redux-observable doesn't provide the right hook to handle the error

@jayphelps

This comment has been minimized.

Show comment
Hide comment
@jayphelps

jayphelps Mar 1, 2017

Member

@mmcgahan oh I see, technically redux-observable is not intended to manage your reducers and any exceptions they may throw. 😄 You can do that yourself with some third party middleware or make your own pretty easily:

http://jsbin.com/xefopiw/edit?js,console

Just make sure to put it before the redux-observable middleware (or any other one really) because otherwise the errors won't be caught--it needs to be the first.

Member

jayphelps commented Mar 1, 2017

@mmcgahan oh I see, technically redux-observable is not intended to manage your reducers and any exceptions they may throw. 😄 You can do that yourself with some third party middleware or make your own pretty easily:

http://jsbin.com/xefopiw/edit?js,console

Just make sure to put it before the redux-observable middleware (or any other one really) because otherwise the errors won't be caught--it needs to be the first.

@jayphelps

This comment has been minimized.

Show comment
Hide comment
@jayphelps

jayphelps May 24, 2017

Member

@gorshkov-leonid ah yes, your example shows what I needed. Indeed you need to isolate your Observable chains, otherwise you allow that error to propagate out which terminates the action$.ofType and any parent epic (like your root epic) until it either reaches a catch or bubbles all the way up and no epic is listening at all.

This is an important thing to learn about RxJS, how observable chains actually work--by chaining subscriptions. It will help make Rx seem less like magic.

It's actually pretty analogous to the way errors (exceptions) propagate in normal functions

pseudo functional code analogy (NOT RxJS)

const doSomething = () => {
  throw 'some error'
};

const somethingEpic(actions, dispatch) => {
  while (actions.length) {
    const action = actions.shift();

    if (action.type === 'SOMETHING') {
      // isolate our "inner mapping logic"
      // so that errors don't stop us from listening
      try {
        const response = doSomething();
        dispatch({
          type: 'SOMETHING_SUCCESS'
        });
      } catch (e) {
        dispatch({
          type: 'SOMETHING_ERROR'
        });
      }
    }
  }
};
Member

jayphelps commented May 24, 2017

@gorshkov-leonid ah yes, your example shows what I needed. Indeed you need to isolate your Observable chains, otherwise you allow that error to propagate out which terminates the action$.ofType and any parent epic (like your root epic) until it either reaches a catch or bubbles all the way up and no epic is listening at all.

This is an important thing to learn about RxJS, how observable chains actually work--by chaining subscriptions. It will help make Rx seem less like magic.

It's actually pretty analogous to the way errors (exceptions) propagate in normal functions

pseudo functional code analogy (NOT RxJS)

const doSomething = () => {
  throw 'some error'
};

const somethingEpic(actions, dispatch) => {
  while (actions.length) {
    const action = actions.shift();

    if (action.type === 'SOMETHING') {
      // isolate our "inner mapping logic"
      // so that errors don't stop us from listening
      try {
        const response = doSomething();
        dispatch({
          type: 'SOMETHING_SUCCESS'
        });
      } catch (e) {
        dispatch({
          type: 'SOMETHING_ERROR'
        });
      }
    }
  }
};
@gorshkov-leonid

This comment has been minimized.

Show comment
Hide comment
@gorshkov-leonid

gorshkov-leonid May 24, 2017

Interesting analogy. I saw the code of retry operation. After error handler will make new subscription. Is it possible to extend this idea? Developer can sets uncatchedErrorsHandler, which provides ability to decide how to handle errors. But subscription in any case will be restored.... Is it fantastic?

gorshkov-leonid commented May 24, 2017

Interesting analogy. I saw the code of retry operation. After error handler will make new subscription. Is it possible to extend this idea? Developer can sets uncatchedErrorsHandler, which provides ability to decide how to handle errors. But subscription in any case will be restored.... Is it fantastic?

@jayphelps

This comment has been minimized.

Show comment
Hide comment
@jayphelps

jayphelps May 24, 2017

Member

@gorshkov-leonid not sure I follow. Can you provide pseudo code for what you mean?

Member

jayphelps commented May 24, 2017

@gorshkov-leonid not sure I follow. Can you provide pseudo code for what you mean?

@braco

This comment has been minimized.

Show comment
Hide comment
@braco

braco May 25, 2017

@jayphelps

Indeed you need to isolate your Observable chains, otherwise you allow that error to propagate out which terminates the action$.ofType and any parent epic (like your root epic) until it either reaches a catch or bubbles all the way up and no epic is listening at all.

Ahhhhh, fuck. Noob here. This is a pretty brutal open door and should probably be foregrounded more heavily in the documentation and have a noisier output in development mode, don't you think? I was combining epics from several files and tracked the problem down to an error in an unrelated epic. At the very least, maybe just "unhandled exception" in console.warn?

braco commented May 25, 2017

@jayphelps

Indeed you need to isolate your Observable chains, otherwise you allow that error to propagate out which terminates the action$.ofType and any parent epic (like your root epic) until it either reaches a catch or bubbles all the way up and no epic is listening at all.

Ahhhhh, fuck. Noob here. This is a pretty brutal open door and should probably be foregrounded more heavily in the documentation and have a noisier output in development mode, don't you think? I was combining epics from several files and tracked the problem down to an error in an unrelated epic. At the very least, maybe just "unhandled exception" in console.warn?

@jayphelps

This comment has been minimized.

Show comment
Hide comment
@jayphelps

jayphelps May 25, 2017

Member

@braco it's not clear how we could improve developer experience or documentation around this without trying to teach them RxJS, which our docs currently try to warn people away from using redux-observable if they don't know Rx. Any suggestions?

At the very least, maybe just "unhandled exception" in console.warn?

Hmm. This should already happen, unless you swallow the error somewhere. redux-observable nor Rx will swallow errors silently, unless you do so yourself.

Member

jayphelps commented May 25, 2017

@braco it's not clear how we could improve developer experience or documentation around this without trying to teach them RxJS, which our docs currently try to warn people away from using redux-observable if they don't know Rx. Any suggestions?

At the very least, maybe just "unhandled exception" in console.warn?

Hmm. This should already happen, unless you swallow the error somewhere. redux-observable nor Rx will swallow errors silently, unless you do so yourself.

@braco

This comment has been minimized.

Show comment
Hide comment
@braco

braco May 25, 2017

redux-observable nor Rx will swallow errors silently, unless you do so yourself.

Wasn't intentionally swallowing any errors, will check on that. Thank you. If an error can be suppressed and still propagate, I wonder if something like redux-saga's warning would be a possible addition.

This thread touches on hand holding vs Rx convention, but since this seems more about redux/sugar, why not present some optional safety?

// Plural since each epic is its own root
rootEpics([epics], config)

isolate: true // Children can't propagate errors
restartOnError: true, // Like this
throwOnError: true // Like this

@jayphelps, thanks for the patience you seem to express toward everyone, it's noticed and appreciated.

braco commented May 25, 2017

redux-observable nor Rx will swallow errors silently, unless you do so yourself.

Wasn't intentionally swallowing any errors, will check on that. Thank you. If an error can be suppressed and still propagate, I wonder if something like redux-saga's warning would be a possible addition.

This thread touches on hand holding vs Rx convention, but since this seems more about redux/sugar, why not present some optional safety?

// Plural since each epic is its own root
rootEpics([epics], config)

isolate: true // Children can't propagate errors
restartOnError: true, // Like this
throwOnError: true // Like this

@jayphelps, thanks for the patience you seem to express toward everyone, it's noticed and appreciated.

@jayphelps

This comment has been minimized.

Show comment
Hide comment
@jayphelps

jayphelps May 25, 2017

Member

If an error can be suppressed and still propagate, I wonder if something like redux-saga's warning would be a possible addition.

I'm not familiar with that particular part of redux-saga. Can you maybe describe how it would work in redux-observable land? If it's similar to your comment below, you can ignore this question.

This thread touches on hand holding vs Rx convention, but since this seems more about redux/sugar, why not present some optional safety?

Some patterns have been discussed above, including the possibility of providing some by default, but the latest consensus seemed to be that most preferred redux-observable let users decide with their own patterns--but I think documenting these various patterns in the Recipes section would be super super helpful, I just haven't had time. 😄

That said, I don't have strong opinions on this as long as it remains flexible still (e.g. opt out of default error handling). Maybe you can convince the others here who have opposed? hehe. For me personally, the beautiful thing about redux-observable is precisely that is just basically glue between redux + RxJS, nothing more. Low API surface, less likely to have bugs, patterns you learn with RxJS are applicable here and stay with you forever even after you leave redux.

thanks for the patience you seem to express toward everyone, it's noticed and appreciated.

You're welcome!💃 That's for being so understanding.

Member

jayphelps commented May 25, 2017

If an error can be suppressed and still propagate, I wonder if something like redux-saga's warning would be a possible addition.

I'm not familiar with that particular part of redux-saga. Can you maybe describe how it would work in redux-observable land? If it's similar to your comment below, you can ignore this question.

This thread touches on hand holding vs Rx convention, but since this seems more about redux/sugar, why not present some optional safety?

Some patterns have been discussed above, including the possibility of providing some by default, but the latest consensus seemed to be that most preferred redux-observable let users decide with their own patterns--but I think documenting these various patterns in the Recipes section would be super super helpful, I just haven't had time. 😄

That said, I don't have strong opinions on this as long as it remains flexible still (e.g. opt out of default error handling). Maybe you can convince the others here who have opposed? hehe. For me personally, the beautiful thing about redux-observable is precisely that is just basically glue between redux + RxJS, nothing more. Low API surface, less likely to have bugs, patterns you learn with RxJS are applicable here and stay with you forever even after you leave redux.

thanks for the patience you seem to express toward everyone, it's noticed and appreciated.

You're welcome!💃 That's for being so understanding.

@gorshkov-leonid

This comment has been minimized.

Show comment
Hide comment
@gorshkov-leonid

gorshkov-leonid May 25, 2017

I do not understand the implementation. But I can agree with one point. Yes, the main criterion to which one should strive is safety.
Ah yes I noticed this issue in "redux-observable", I think solution can be build on top of rxjs feature. How to do it? If chain unsubscribe on error somewhere inside rxjs, what can we do?

gorshkov-leonid commented May 25, 2017

I do not understand the implementation. But I can agree with one point. Yes, the main criterion to which one should strive is safety.
Ah yes I noticed this issue in "redux-observable", I think solution can be build on top of rxjs feature. How to do it? If chain unsubscribe on error somewhere inside rxjs, what can we do?

@braco

This comment has been minimized.

Show comment
Hide comment
@braco

braco May 25, 2017

@jayphelps

Can you maybe describe how [redux-saga's warning] would work in redux-observable land?

In dev mode, if an epic failed or was cancelled from propagation, there would be a console.info note. Every epic would announce its own cancellation. As the error propagated, you'd get epic {name} was cancelled or epic {name} ended for each epic, and probably root epic was cancelled. Would also be great if there was a mini-stack if possible: epic Bar was cancelled (origin: Foo). The equivalent feature in redux-saga was vital for debugging.

If unadorned RxJS is important, noisier dev would be a reasonable compromise imo, and would go a long way to guide users in self-diagnosis. Flow control is nasty business, and I would assume this library will usually be the gateway drug to RxJS instead of the opposite.

I do think some default safety is warranted given the specificity in Redux convention. Even if that was just a default warnUncaughtErrors: true and optional throwUncaughtErrors: true that wrapped root and reducer-level epics like @jtomaszewski's code above.

You could also sneak in some RTFM hints in console output like React is wont to do: "epic Foo was cancelled. Make sure to catch errors, as .... see: http://reactivex.io/rxjs/...", but not sure how generalized that can be since you're library agnostic?

Which of you oppositional interlocutors needs convincing?

braco commented May 25, 2017

@jayphelps

Can you maybe describe how [redux-saga's warning] would work in redux-observable land?

In dev mode, if an epic failed or was cancelled from propagation, there would be a console.info note. Every epic would announce its own cancellation. As the error propagated, you'd get epic {name} was cancelled or epic {name} ended for each epic, and probably root epic was cancelled. Would also be great if there was a mini-stack if possible: epic Bar was cancelled (origin: Foo). The equivalent feature in redux-saga was vital for debugging.

If unadorned RxJS is important, noisier dev would be a reasonable compromise imo, and would go a long way to guide users in self-diagnosis. Flow control is nasty business, and I would assume this library will usually be the gateway drug to RxJS instead of the opposite.

I do think some default safety is warranted given the specificity in Redux convention. Even if that was just a default warnUncaughtErrors: true and optional throwUncaughtErrors: true that wrapped root and reducer-level epics like @jtomaszewski's code above.

You could also sneak in some RTFM hints in console output like React is wont to do: "epic Foo was cancelled. Make sure to catch errors, as .... see: http://reactivex.io/rxjs/...", but not sure how generalized that can be since you're library agnostic?

Which of you oppositional interlocutors needs convincing?

@jayphelps

This comment has been minimized.

Show comment
Hide comment
@jayphelps

jayphelps May 25, 2017

Member

is @braco and @gorshkov-leonid the same person or different? It's hard to tell 😄

Member

jayphelps commented May 25, 2017

is @braco and @gorshkov-leonid the same person or different? It's hard to tell 😄

@braco

This comment has been minimized.

Show comment
Hide comment
@braco

braco May 25, 2017

😄 We're 2 cautionary ghosts, as in A Christmas Carol. Ghost of noob future is coming. Ignore us at your own risk

braco commented May 25, 2017

😄 We're 2 cautionary ghosts, as in A Christmas Carol. Ghost of noob future is coming. Ignore us at your own risk

@jayphelps

This comment has been minimized.

Show comment
Hide comment
@jayphelps

jayphelps May 26, 2017

Member

In dev mode, if an epic failed or was cancelled from propagation, there would be a console.info note. Every epic would announce its own cancellation. As the error propagated, you'd get epic {name} was cancelled or epic {name} ended for each epic, and probably root epic was cancelled. It was vital with redux-saga to catch problems. Would also be great if there was a mini-stack if possible: epic Bar was cancelled (origin: Foo).

That seems great, generally. It is a good compromise to use console.error to log about the epics terminating to help debug, but still bubble and cascade the error to let people handle it how they choose.

Anyone have any objections?

Member

jayphelps commented May 26, 2017

In dev mode, if an epic failed or was cancelled from propagation, there would be a console.info note. Every epic would announce its own cancellation. As the error propagated, you'd get epic {name} was cancelled or epic {name} ended for each epic, and probably root epic was cancelled. It was vital with redux-saga to catch problems. Would also be great if there was a mini-stack if possible: epic Bar was cancelled (origin: Foo).

That seems great, generally. It is a good compromise to use console.error to log about the epics terminating to help debug, but still bubble and cascade the error to let people handle it how they choose.

Anyone have any objections?

@BerkeleyTrue

This comment has been minimized.

Show comment
Hide comment
@BerkeleyTrue

BerkeleyTrue May 26, 2017

Collaborator

@jayphelps Where would this happen, combineEpics or createEpicMiddleware?

Collaborator

BerkeleyTrue commented May 26, 2017

@jayphelps Where would this happen, combineEpics or createEpicMiddleware?

@jayphelps

This comment has been minimized.

Show comment
Hide comment
@jayphelps

jayphelps May 26, 2017

Member

@BerkeleyTrue depends. I experimented a bit and if someone is using combineEpics we can use it to annotate the error with the originating epic's name but still bubble it up and then console.error it in the middleware if it reaches that, with something like "all epics terminated due to uncaught error in myAwesomeEpic" but still let the error bubble further, naturally resulting in window.onerror triggering and the true error shown in the console.

Member

jayphelps commented May 26, 2017

@BerkeleyTrue depends. I experimented a bit and if someone is using combineEpics we can use it to annotate the error with the originating epic's name but still bubble it up and then console.error it in the middleware if it reaches that, with something like "all epics terminated due to uncaught error in myAwesomeEpic" but still let the error bubble further, naturally resulting in window.onerror triggering and the true error shown in the console.

@jayphelps

This comment has been minimized.

Show comment
Hide comment
@jayphelps

jayphelps May 26, 2017

Member

I'm hesitant to also do console.error inside combineEpics itself because

  • if it bubbled to the root, there would be three total errors in the console
    1. original (originated in epic, but bubbled all the way until natural window.onerror)
    2. X epic terminated (from combineEpics)
    3. all epics terminated because of X epic. (from middleware)
  • just because an epic terminated from the perspective of combineEpics doesn't mean they won't catch the error before it reaches the root epic, so the error message would be annoying to some, especially if they catch in a parent epic by design.

Although certainly someone could be unknowingly catching and swallowing errors, so they wouldn't see anything in the console, but I don't currently feel we should save people from themselves at the expense of making others overly annoyed. Maybe I can be convinced though..Or a flag to combineEpics could be an option to opt into more strict logging at that level.

Member

jayphelps commented May 26, 2017

I'm hesitant to also do console.error inside combineEpics itself because

  • if it bubbled to the root, there would be three total errors in the console
    1. original (originated in epic, but bubbled all the way until natural window.onerror)
    2. X epic terminated (from combineEpics)
    3. all epics terminated because of X epic. (from middleware)
  • just because an epic terminated from the perspective of combineEpics doesn't mean they won't catch the error before it reaches the root epic, so the error message would be annoying to some, especially if they catch in a parent epic by design.

Although certainly someone could be unknowingly catching and swallowing errors, so they wouldn't see anything in the console, but I don't currently feel we should save people from themselves at the expense of making others overly annoyed. Maybe I can be convinced though..Or a flag to combineEpics could be an option to opt into more strict logging at that level.

@BerkeleyTrue

This comment has been minimized.

Show comment
Hide comment
@BerkeleyTrue

BerkeleyTrue May 26, 2017

Collaborator

I like the idea of annotating the error message in combineEpics.

import { merge } from 'rxjs/observable/merge';

import { errorSymbol } from './error-symbol.js';

function annotate(err, epic) {
  const epicName = epic.name || 'Anon Epic';
  // prevent error source from overriding
  if (err[errorSymbol]) {
    // could even be an additive process
    // like so: err[errorSymbol] = `${epicName}(${err[errorSymbol]})`;
    return err;
  }
  err[errorSymbol] = epicName;
  return err;
};
/**
  Merges all epics into a single one.
 */
export const combineEpics = (...epics) => (...args) =>
  merge(
    ...epics.map(epic => {
      const output$ = epic(...args);
      if (!output$) {
        throw new TypeError(`combineEpics: one of the provided Epics "${epic.name || '<anonymous>'}" does not return a stream. Double check you\'re not missing a return statement!`);
      }
      return output$.catch(err => [annotate(err, epic.name)]);
    })
  );
Collaborator

BerkeleyTrue commented May 26, 2017

I like the idea of annotating the error message in combineEpics.

import { merge } from 'rxjs/observable/merge';

import { errorSymbol } from './error-symbol.js';

function annotate(err, epic) {
  const epicName = epic.name || 'Anon Epic';
  // prevent error source from overriding
  if (err[errorSymbol]) {
    // could even be an additive process
    // like so: err[errorSymbol] = `${epicName}(${err[errorSymbol]})`;
    return err;
  }
  err[errorSymbol] = epicName;
  return err;
};
/**
  Merges all epics into a single one.
 */
export const combineEpics = (...epics) => (...args) =>
  merge(
    ...epics.map(epic => {
      const output$ = epic(...args);
      if (!output$) {
        throw new TypeError(`combineEpics: one of the provided Epics "${epic.name || '<anonymous>'}" does not return a stream. Double check you\'re not missing a return statement!`);
      }
      return output$.catch(err => [annotate(err, epic.name)]);
    })
  );
@BerkeleyTrue

This comment has been minimized.

Show comment
Hide comment
@BerkeleyTrue

BerkeleyTrue May 26, 2017

Collaborator

@jayphelps I've also been thinking about a different approach that could help out during development.

What do you think of the addition of a new function, say createEpic, That will take an epic and add a bunch of additional checks to the epic and in production default to an identity function?

Collaborator

BerkeleyTrue commented May 26, 2017

@jayphelps I've also been thinking about a different approach that could help out during development.

What do you think of the addition of a new function, say createEpic, That will take an epic and add a bunch of additional checks to the epic and in production default to an identity function?

@jayphelps

This comment has been minimized.

Show comment
Hide comment
@jayphelps

jayphelps May 31, 2017

Member

@BerkeleyTrue I've played with similar things but nothing ended up being super useful to me but I'm biased cause I know how everything works. 🤡 having more meaningful checks/errors would be helpful to newbies I imagine so it's worth considering. If you have time to put together a proof of concept, feel free to open an RFC or even PR if it's easy.

I will say I think we'd still allow regular functions-as-epics as it is today. For me, one of the major benefits of the existing approach is that really it's just a plain old function so you can do normal functional things for custom abstractions, etc. and it's simpler to learn IMO. But I'll try to keep an open mind with any proposals.

The original goal was minimal amount of glue between redux + RxJS, with the expectation that the community might naturally create additional, more opinionated abstractions that people could choose to adopt or not. But if something is clearly better, let's do it.

Member

jayphelps commented May 31, 2017

@BerkeleyTrue I've played with similar things but nothing ended up being super useful to me but I'm biased cause I know how everything works. 🤡 having more meaningful checks/errors would be helpful to newbies I imagine so it's worth considering. If you have time to put together a proof of concept, feel free to open an RFC or even PR if it's easy.

I will say I think we'd still allow regular functions-as-epics as it is today. For me, one of the major benefits of the existing approach is that really it's just a plain old function so you can do normal functional things for custom abstractions, etc. and it's simpler to learn IMO. But I'll try to keep an open mind with any proposals.

The original goal was minimal amount of glue between redux + RxJS, with the expectation that the community might naturally create additional, more opinionated abstractions that people could choose to adopt or not. But if something is clearly better, let's do it.

@meticoeus

This comment has been minimized.

Show comment
Hide comment
@meticoeus

meticoeus Jun 1, 2017

I think anything redux-observable does for you to catch errors or report on errors should be opt-in and well documented. I prefer the flexibility of RxJS to handle any types of errors appropriate to the situation that might throw them.

@braco

If unadorned RxJS is important, noisier dev would be a reasonable compromise imo, and would go a long way to guide users in self-diagnosis. Flow control is nasty business, and I would assume this library will usually be the gateway drug to RxJS instead of the opposite.

As long as this library is a minimalistic glue layer between redux and RxJS (which I hope it remains) I think users are going to find it much easier to learn RxJS without the added complexity of interacting with redux and then learning to use them together, rather than using redux-observable as the gateway direction.

meticoeus commented Jun 1, 2017

I think anything redux-observable does for you to catch errors or report on errors should be opt-in and well documented. I prefer the flexibility of RxJS to handle any types of errors appropriate to the situation that might throw them.

@braco

If unadorned RxJS is important, noisier dev would be a reasonable compromise imo, and would go a long way to guide users in self-diagnosis. Flow control is nasty business, and I would assume this library will usually be the gateway drug to RxJS instead of the opposite.

As long as this library is a minimalistic glue layer between redux and RxJS (which I hope it remains) I think users are going to find it much easier to learn RxJS without the added complexity of interacting with redux and then learning to use them together, rather than using redux-observable as the gateway direction.

@jayphelps

This comment has been minimized.

Show comment
Hide comment
@jayphelps

jayphelps Jun 1, 2017

Member

@meticoeus Just to be clear, would you be opposed to the default (not opt-in) behavior of: errors that caused the termination of every epic were console.error logged, but not caught? So a friendly message would let you know all epics were terminated, but the original error itself would still get caught by things like window.onerror etc and also show up in the console. The separate console.error will contain info the normal error won't, like the "all epics have terminated due to an uncaught error in somethingEpic"

I think this would be useful in every situation, even for Rx pros.

Member

jayphelps commented Jun 1, 2017

@meticoeus Just to be clear, would you be opposed to the default (not opt-in) behavior of: errors that caused the termination of every epic were console.error logged, but not caught? So a friendly message would let you know all epics were terminated, but the original error itself would still get caught by things like window.onerror etc and also show up in the console. The separate console.error will contain info the normal error won't, like the "all epics have terminated due to an uncaught error in somethingEpic"

I think this would be useful in every situation, even for Rx pros.

@meticoeus

This comment has been minimized.

Show comment
Hide comment
@meticoeus

meticoeus Jun 1, 2017

@jayphelps That sounds reasonable.

meticoeus commented Jun 1, 2017

@jayphelps That sounds reasonable.

@braco

This comment has been minimized.

Show comment
Hide comment
@braco

braco Jun 20, 2017

If this helps any rank amateurs in the meantime, I'm doing this:

https://gist.github.com/braco/28dc2006e9ce04a004d2d9c4f5b9a4b6

Convention here is

epics.js

import reducerName1 from 'reducerName1/epics';
...

export default wrapEpics({ reducerName1 });

reducerName1/epics.js

export const epicName = epic$ => ...
...

Then you get debug messages like reducerName/epicName errored with ... and reducerName/epicName exited

braco commented Jun 20, 2017

If this helps any rank amateurs in the meantime, I'm doing this:

https://gist.github.com/braco/28dc2006e9ce04a004d2d9c4f5b9a4b6

Convention here is

epics.js

import reducerName1 from 'reducerName1/epics';
...

export default wrapEpics({ reducerName1 });

reducerName1/epics.js

export const epicName = epic$ => ...
...

Then you get debug messages like reducerName/epicName errored with ... and reducerName/epicName exited

@BetterCallSky

This comment has been minimized.

Show comment
Hide comment
@BetterCallSky

BetterCallSky Sep 13, 2017

This is my wrapper for unit testing (Jest)

const wrapEpic = epic => (...args) =>
  epic(...args).catch((error) => {
    console.error(error);
    expect.assertions(Infinity);
  });

It logs an error with correct stack trace, and makes sure the test always fails.
Adding extra throw error doesn't make any difference.

BetterCallSky commented Sep 13, 2017

This is my wrapper for unit testing (Jest)

const wrapEpic = epic => (...args) =>
  epic(...args).catch((error) => {
    console.error(error);
    expect.assertions(Infinity);
  });

It logs an error with correct stack trace, and makes sure the test always fails.
Adding extra throw error doesn't make any difference.

@nhducit

This comment has been minimized.

Show comment
Hide comment
@nhducit

nhducit Dec 6, 2017

@jayphelps I think we should mention this issue in Readme and provide a workaround solution.

nhducit commented Dec 6, 2017

@jayphelps I think we should mention this issue in Readme and provide a workaround solution.

@hally9k

This comment has been minimized.

Show comment
Hide comment
@hally9k

hally9k Jun 12, 2018

@jayphelps I am trying to use the adapter option in createEpicMiddleware to add a catchError to every epic in my epic tree. I'm getting a bit confused about how the pieces fit together, a little help? Is this approach possible? It seems like it could be a cleaner solution than the above approach...

hally9k commented Jun 12, 2018

@jayphelps I am trying to use the adapter option in createEpicMiddleware to add a catchError to every epic in my epic tree. I'm getting a bit confused about how the pieces fit together, a little help? Is this approach possible? It seems like it could be a cleaner solution than the above approach...

@jayphelps

This comment has been minimized.

Show comment
Hide comment
@jayphelps

jayphelps Jun 12, 2018

Member

@hally9k I would recommend instead using function composition in your root Epic:

Assuming rxjs v6 and redux-observable 1.0.0-beta.1:

// TODO: replace doSomethingAndReturnObservable
const combineEpicsAndCatchErrors = (...epics) =>
  return (action$, state$) => {
    epics = epics.map(epic => (action$, state$) => epic(action$, state$).pipe(
      catchError(e => doSomethingAndReturnObservable(e))
    ));
    return combineEpics(...epics)(action$, state$);
  };
};

const rootEpic = combineEpicsAndCatchErrors(...epics);

If you don't need to do it individually for each epic, it's easier to just do it to the rootEpic (but has different behavior):

const rootEpic = (action$, state$) =>
  combineEpics(...epics)(action$, state$).pipe(
    catchError(e => doSomethingAndReturnObservable(e))
  );
Member

jayphelps commented Jun 12, 2018

@hally9k I would recommend instead using function composition in your root Epic:

Assuming rxjs v6 and redux-observable 1.0.0-beta.1:

// TODO: replace doSomethingAndReturnObservable
const combineEpicsAndCatchErrors = (...epics) =>
  return (action$, state$) => {
    epics = epics.map(epic => (action$, state$) => epic(action$, state$).pipe(
      catchError(e => doSomethingAndReturnObservable(e))
    ));
    return combineEpics(...epics)(action$, state$);
  };
};

const rootEpic = combineEpicsAndCatchErrors(...epics);

If you don't need to do it individually for each epic, it's easier to just do it to the rootEpic (but has different behavior):

const rootEpic = (action$, state$) =>
  combineEpics(...epics)(action$, state$).pipe(
    catchError(e => doSomethingAndReturnObservable(e))
  );
@GodefroyClair

This comment has been minimized.

Show comment
Hide comment
@GodefroyClair

GodefroyClair Jul 7, 2018

This is a quite long discussion to swallow... Any place where we can find a sumup of the recommended practices?

GodefroyClair commented Jul 7, 2018

This is a quite long discussion to swallow... Any place where we can find a sumup of the recommended practices?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment