Safe Apollo Reducers that can't crash the store

[rricard]

TODO:

• If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes)
• Make sure all of the significant new logic is covered by tests
• Rebase your changes on master so that they can be merged easily
• Make sure all tests and linter rules pass
• Update CHANGELOG.md with your change
• Add your name and email to the AUTHORS file (optional)
• If this was a change that affects the external API, update the docs and post a link to the PR in the discussion

We prevent throwing inside the reducer by wrapping it in a try/catch block. The error will still be reported to the user.

[rricard]

@helfer here it is finally!

[helfer]

This is neat, thanks a lot! Do you think you could come up with a test that breaks the resolvers and exercises the code in the catch block? If not, I think we should add a console.warn statement so that it's possible to debug these errors (otherwise they will essentially be silently swallowed).

It would also be useful to have at least one example of something that this code helps fix in the wild. I know it's good for stability, but we also have to worry about debuggability, and right now I'm leaning a bit more towards debuggability.

[helfer]

@rricard let me know if you have time to take another look at this. I would really like to merge it for the 0.6 release which will be in the next few days.

[rricard]

@helfer can that wait the week end ? or I can try to take a look tonight if 0.6 is really coming soon ...

[rricard]

But I agree, I have to find some way to ensure it will not swallow errors

[helfer]

@rricard by the weekend is fine, no rush!

[rricard]

@helfer: so the main uses I had of this catcher are now gone since the legacy diffing is gone. So I had to find an another way to trigger a throw in the reducer. What I test here works, moreover I ensure the error is not swallowed.

[rricard]

So for me this error catcher shouldn't be triggered once but when it does, that means something is going wrong in Apollo, an error is likely to be uncatched. And in this case, even if something is really wrong, that doesn't mean we should let redux crash. So I would be for adding a big warning in the console printing the stacktrace asking for the user to open up a bug report about it if the reducer crashes. Note that a reducer crash can happen if we throw (what the legacy query diffing was doing) or if we end up hitting an undefined prop. Both situations need to be catched and that's we're doing here!

[helfer]

@rricard awesome! Once you add the warning, we're good to merge here.

[helfer]

@rricard I just tried adding a warning, which made me realize that errors that are not Apollo Client internal errors are actually possible in the reducers, which means that the error is more likely due to code that the user wrote than due to code that we wrote. Concrete examples:

• dataIdFromObject that starts with a $ character.
• replacing a real id node in the store with a generated id node

For that reason I will not add a warning and simply merge the PR as it is now 🎉

[rricard]

@helfer oh ok, didn't see this one but you're right! Thanks! I was going to handle that this WE but glad you took a look before ;)