Always log reducers exception #263

[Jerry-Hong]

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

#263

[jayphelps]

Hey @Jerry-Hong, thanks for the PR!!

I'm gonna make some minor adjustments to make sure that console.error exists but otherwise I appreciate the initiative.

[jayphelps]

hmm just realized an issue that is probably a deal breaker to this: console.error does not report to window.onerror so if we catch all these errors people who are using window.error to report errors to some logging service might not work correctly. I imagine most of them override console.error too to gather them, but I don't think we should make that assumption.

The alternative I'm looking into is using the old async rethrow setTimeout(() => { throw err; }) which isn't ideal but any other solution I can think of like window.dispatchEvent(new ErrorEvent(...)) or Promise.reject(err) have worse caveats.

Here's a first crack at it: #380

Any objections?

[jayphelps]

I also remembered that catching all errors at this place prevents some errors that would have otherwise bubbled all the way back to the original place someone called store.dispatch() outside of redux-observable or any epics. I've known this particular behavior for a while but it's pretty non-obvious to many people so I'm torn on whether this is a good thing or not. Someone might be relying on it?

e.g. if your epic listens for an action and then synchronously emits another one the error would have historically bubbled all the way back up:

https://jsbin.com/dagoluw/edit?js,console

// synchronously listen and emit
const pingEpic = action$ =>
  action$.ofType('PING')
    .mapTo({ type: 'PONG' });

const pingReducer = (state = { isPinging: false }, action) => {
  switch (action.type) {
    case 'PING':
      return { isPinging: true };

    case 'PONG':
      throw new Error('BAD'); // <----------------------------------- error thrown

    default:
      return state;
  }
};

// pretend this is in your UI or anywhere else in your app
try {
  store.dispatch({ type: 'PING' });
} catch (e) {
  // error bubbles all the way back up to here!
  console.log('I CAUGHT THIS: ', e.message);
}

[MrOrz]

+1 for async rethrow, it helps collecting error logs and send them through error tracking systems like Rollbar.

As for the scenario depicted in #379 (comment) , try-catch on store.dispatch is not possible in RxJS 5.1.0 ~ 5.4.x. Please refer to https://jsbin.com/wutezibaxo/1/edit?html,js,console (this is actually taken from @jayphelps 's previous comment )

The main reason for this breakage is that RxJS is swallowing the error and waiting for TC39 to come up with a solution. For the time being, I think @Jerry-Hong 's PR and is an adequate temporary solution because:

1. We often make mistakes in redux reducers, notifying the developer about the error is important
2. console.error has no side effects other than printing words in the console, and people can hardly build stuff upon it, so when we remove this mechanism in the future (when TC39 resolves the issue), it may not be a lot of pain.

Async re-throw, on the other hand, comes with the assumption that users may build an error logging mechanism around it. I also consider it adequate, as long as we document that things may change after TC39 has made up their mind.

Update: I compared the 2 jsbin, and found that rx.js 5.5 actually throws, only 5.1~5.4 swallows
😕
Maybe just upgrading to RxJS 5.5 is the more proper solution?

[Jerry-Hong]

Async rethrow error is good to me 👍

[jayphelps]

@MrOrz Thanks for chiming in! Glad some people are helping sanity check things. Can you clarify your position? At first it sounded like you async rethrow, then you wanted console.error, then it sounded like you thought async rethrow, then at the end that neither are needed? 😆 sorry, it's been a long day for me 😄 rereading it I'm guessing console.error "it may not be a lot of pain" was meant to be that it will cause a lot of pain?

To clarify btw this bug still exists in the latest 5.5.3. There has been several attempts to fix it but each one only fixed particular cases of it. It's still possible to run into it, here's one: https://jsbin.com/lawipop/1/edit?js,console

and waiting for TC39 to come up with a solution.

I'm not on the core team anymore so I'm not speaking for them but it's my understanding that it hasn't been fixed because no one has been able to find the one solution "to rule them all", but not because anyone's waiting for TC39 to figure out the Observable swallowing behavior. So it's definitely a bug in v5. v6 may or may not have swallowing behavior depending on its release timeline but if it does it's almost certainly going to present them in the console/window.onerror somehow, similar to how Promises swallow but still show up in the console for debugging. My comment here goes into more detail. for anyone else who's curious. tl;dr the primary reason people hate complete swallowing is because it doesn't show up in the dev console and regardless of what goes down with TC39 and swallowing it seems 99% certain that the error will at the very least show up in the console, even if it's technically "swallowed".

[jayphelps]

I've been going deeper down the rabbit hole. The core team already in fact merged a PR for the new v6 behavior of not rethrowing errors aka swallowing ReactiveX/rxjs#3062 Just like Promises it will still report the error into the console because it uses the setTimeout rethrow trick. Technically that means it uses HostReportErrors (per the current proposed Observable spec) but Promises use HostPromiseRejectionTracker, the difference mostly being that promises do not trigger window.onerror for whatever reason and instead have the unhandledrejection event.

This is mostly an FYI to the incoming changes. This is still a bug in v5, especially since it doesn't report anything in the console. I've spent a bunch of time trying to figure it out but so far everything I try breaks something else.

[MrOrz]

@jayphelps thank you so much for digging into this deeply.
My position was, it's ok for me as long as I get notified of the errors, therefore console.error, async re-throw or even upgrading to 5.5 seems all good to me :D (And thanks for letting me know that ugprading to 5.5 still has some pitfalls, it's super informative!)

I totally agree that in v5 it can be considered as a bug, and we might not get reliable error messages from RxJS before everything settles down in v6.

If your concern is that @Jerry-Hong 's try-catch in subscribe() may break something like the scenario you depicted here, how about doing a (synchronous) re-throw the error in Jerry's catch block, right after console.error? For those who uses try-catch around store.dispatch, they still get the stack trace back to the source (and an extra console.error for free (?)), and for those who don't, they can get notified by console.error.

If an extra console.error seems too dreadful, maybe we can use console.warn instead.

[jayphelps]

@MrOrz I really appreciate the thanks! 🕺 I've been digging into this so deeply but it's pretty confusing with so many moving parts lol.

If an extra console.error seems too dreadful, maybe we can use console.warn instead.

I agree. I'm also leaning to the extra console.error, but gonna give it a few days to settle. It's annoying, but so far seems like the safest option for everyone and two errors is better than no errors! Assuming that I can't figure out the rxjs bug itself before 😢 everything I try breaks something else.

[mr-pinzhang]

@jayphelps thx for really careful concerns about error handler, those can be complicated after all, appreciate your thinkings.
but as far as right now, how about pass the onSubscribe (any name will be fine : P) for subscribe callback with store, dispatch... params. and then developer can choose which way handle error as they want or just dispatch actions like the default behavior.
just an assumption, note my ignorance 😅

[jayphelps]

@bjmin yep that's a possibility too. Will think it over some more.

---

I just submitted a PR upstream to rxjs with a "fix" that seems to work for all the cases I've come across with. ReactiveX/rxjs#3161

I've published it as @jayphelps/rxjs with the version 5.5.3-yolo if anyone would care to give it a spin and let me know if it solves the swallowing issue (and doesn't break anything else of course). Using it though would require knowing how to use webpack aliases which prolly isn't obvious. Or if you want to just temporarily use try it locally you can replace install it then just rename it node_modules/@jayphelps/rxjs to regular node_modules/rxjs.

Don't get your hopes up just yet as fixes like these can be pretty controversial and fearful since so many people rely on rxjs it's easy to fix one thing but cause major issues for a much larger group of people 😆

[mr-pinzhang]

looking forwards 2 next progress!.. thx

[mr-pinzhang]

but I am still wondering when to publish this fix version?

[jayphelps]

@bjmin this particular PR won't make it unfortunately because it would prevent error reporting to window.onerror (see #379 (comment))

[mr-pinzhang]

@jayphelps i see, sorry for misunderstanding. i thought u would publish those fixes first, then wait until the rxjs accept a fix for 5.x version. u r right, that's reasonable.

[mr-pinzhang]

@jayphelps maybe i'll make a fork version and use it privately in my team.

[jayphelps]

FYI rxjs 5.5.6 fixed the swallow issue ReactiveX/rxjs#3161