Added improved method of logging errors

[Sawtaytoes]

• I have read the Contributor Guide
• My commit messages are in conventional-changelog-standard format.

Reason for PR

I noticed combineEpics doesn't do anything to help locate errors. Almost always, when using redux-observable, it's nearly impossible to find errors unless you've got a catchError at the bottom of your pipeline.

In my own Redux-less implementation of Redux-Observable, I added this functionality:

createRootEpic	catchEpicError

Not only does this implementation include a way of logging errors as actions, it also logs them to the console with the name of the Epic that caused the error.

It's super helpful in debugging. combineEpics could definitely benefit from functionality like this.

In case there's a stack available such as the error being an EventError (Node.js), one of my other versions of this operator checks for stack information.

Changes

This method makes it obvious which epic actually threw an error and then throws it again so it proceeds as usual similar to doing a Promise.prototype.catch and throwing the error again so the next .catch picks it up.

Just like when throwing a TypeError, this also utilizes epic.name.

[jayphelps]

Thanks for the PR! <3

I believe console.error still isn't safe to use broadly if we want to support IE 8 and 9--I believe they only provide them if the dev tools are open, and we wouldn't want to not emit an error. Also I don't believe console.error triggers window.onerror unfortunately. We could do setTimeout(() => { throw e; }) as the hacky workaround though maybe?

We'd have to modify the error message string itself though and that feels dirty too.

Overall I'm squeamish and not sure if there's a general purpose approach we can do. Maybe we could get away with providing something in dev-mode only, which seems much more safe? e.g. we rethrow the error, but also provide a console.warn or console.error (when available) that provide more context and a link to a new doc page about various patterns for handling errors?

That would be similar to what I was planning to do here: #94 (comment) except this might have to be done at the combineEpics level so we have epic.name for each one. That might be a dealbreaker because then that would still warn even if someone provided their own default error handler after combineEpics.

Still, we could provide a way to turn off the warning, and/or combineEpics isn't particularly complicated if someone wanted to reimplement it without the warning code.

These sorts of "but what if" issues are why I haven't made any of these yet. Especially since people can pretty easily do it themselves if they'd like, though of course defaults are nice and not everyone realizes how.

I dunno, what do you think about my points?

[jayphelps]

Oh forgot to add: this is mostly to be an unfortunate side effect of how RxJS (and nearly all similar lazy libs) work; the call stack being an internal one rather than your original source location where you declared the chain of operators. Not that we shouldn't consider adding help when we can.

[Sawtaytoes]

I get what you mean. I like the idea of using console.error only when it's available. I agree with the idea of adding an option to limit re-throwing the error or logging if people have their own.

Previous Projects

On the projects I've worked, I never considered adding a catch-all until today (2 years later). I always had trouble figuring out where an error was occurring and this was always trouble when telling people about the benefits of RxJS and Redux-Observable.

Another method I've done is create a catchEpicError wrapper for catchError which takes a string, the epic's name. I put this at the bottom of every single epic and have it log the epic name in the error. I'd usually forget to add this operator, so it wasn't always useful. Plus, it was another step in remembering to name the epic the same as the string passed-in.

Reasoning

That's why I prefer combineEpics doing the error logging. After adding this to my own implementation on side projects, debugging became much simpler. If I had this back when I was using Redux-Observable at work, it would've changed the game for new developers who were completely lost when a random operator started erroring who-knows-where.

There's also the possibility of creating a separate combineEpics function for this purpose or adding more args to combineEpics so error logging is another option. Either way, that's a lot of developer overhead when it should really be the default (my opinion).

If other people are compensating for this bug like you said, I bet they'd rather this solution instead since it's more granular.

Global Error-Catching PR

In your other PR (#94), it appears the goal is making sure Redux-Observable keeps going so an error doesn't kill off a bunch of epics down the chain right? It seems like what you were planning there would work fine even with this PR but does change the functionality quite a bit.

I've had epics fail before and take everything down with them. I've also had epics error and stop working, but everything else worked fine so I wasn't aware of an issue. As long as we can get something in there to show runtime errors, it'll be significantly easier to debug when these issues occur.

[jayphelps]

In your other PR (#94), it appears the goal is making sure Redux-Observable keeps going so an error doesn't kill off a bunch of epics down the chain right? It seems like what you were planning there would work fine even with this PR but does change the functionality quite a bit.

The current consensus was that we would not in fact retry, we would only log the error. See my last comment: #94 (comment)

[Sawtaytoes]

Cool. What's the consensus on this PR? Do I just need to make adding the catchError optional through a setting and we're good or should there be a large discussion presented somewhere?

[evertbouw]

If this is optional and/or only in dev it's fine by me.

[Sawtaytoes]

I'd prefer a config var so, by default, this would be enabled and people could disable it as-needed, but there are some issues doing so:

• combineEpics takes ...epics so you can't just pass in a second arg of options. It's not impossible, but we'd have to search epics for an object (most-likely the last value).
• Another issue, combineEpics is used everywhere. It's not something we create, but something that exists. To add a configurable option, you'd need to attach it to every combineEpics function. To get around this, you'd need have a combineEpics creator function: createEpicCombiner. This is a lot to manage in your project and is a much more difficult upgrade path.

For now, I've simply restricted it to development mode. This is the minimal-changes-needed version which will hold until a v2 where you could get away with API changes.

Unless it's arleady there, we'll need documentation talking about adding process.env.NODE_ENV to each project's Webpack config.

[Sawtaytoes]

If this is good to go, can someone merge it?

[jayphelps]

@Sawtaytoes not good to go yet. Sorry, not sure when I'll have time to dig deep about this and the implications to make a decision. There will likely need to be adjustments but I don't want to keep having your go back and forth until I figure them all out. e.g. typeof console, whether having both errors is too noisy, etc.

[Sawtaytoes]

Have you had a chance to look into this @jayphelps? I don't mind making changes that suit the project.

I'd like to make Redux-Observable something really easy for people to debug as soon as possible. I've already got new buddies starting to use it on their teams and better debugging would be super helpful.

[evertbouw]

Closing old PRs. As always thank you for your contribution.