redux-observable / redux-observable

RxJS middleware for action side effects in Redux using "Epics"
https://redux-observable.js.org
MIT License
7.85k stars 468 forks source link

RFC: default error handling of Epics #94

Open jayphelps opened 7 years ago

jayphelps commented 7 years ago

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 commented 7 years ago

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

jayphelps commented 7 years ago

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.

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.

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 commented 7 years ago

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: https://github.com/yelouafi/redux-saga/issues/570

jayphelps commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

jayphelps commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago
  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 commented 7 years ago

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

salanki commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

@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.

mmcgahan commented 7 years ago

ah that makes a lot of sense, thanks!

gorshkov-leonid commented 7 years ago

@jtomaszewski @jayphelps After catch of error in root-epic-wrapper or in wrapper-middleware - combine latest will not work.... : https://github.com/Reactive-Extensions/RxJS/issues/1036

jayphelps commented 7 years ago

@gorshkov-leonid not sure what you mean--btw that link refers to RxJS v4, v5 is here https://github.com/ReactiveX/rxjs and was a complete rewrite

gorshkov-leonid commented 7 years ago

@jayphelps I not sure too :thinking:
Example with usage of 5.1.1: https://jsfiddle.net/goleon/hq2c15cf/ I emulated 'save' with global error handler and 'load' with error handler inside switch map. May be problem in switch map...? So advice to handle error inside mergeMap seems logical...

As result root handler does not guarantee working condition... Why he is needed then? Is it need to show critical modal dialog with message 'please refresh page'?

jayphelps commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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

braco commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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

braco commented 7 years ago

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

jayphelps commented 7 years ago

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 commented 7 years ago

@jayphelps Where would this happen, combineEpics or createEpicMiddleware?

jayphelps commented 7 years ago

@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 commented 7 years ago

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

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

@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. :clown_face: 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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

@jayphelps That sounds reasonable.

braco commented 6 years ago

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

lstkz commented 6 years ago

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.