Skip to content
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

Log errors thrown in reducer #301

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@2color
Copy link
Contributor

commented May 14, 2019

Fixes #262

@2color 2color changed the title Log errors thrown in reducer [wip] Log errors thrown in reducer May 14, 2019

Fix the store reducer test
The addition of the a catch callback to the promise means that
other callbacks to the promise or derived observable happen in the
next event loop cycle.
Return the observable so Ava waits for it to complete and remove
the unnecessary asyncwip: fix tests.

@2color 2color changed the title [wip] Log errors thrown in reducer Log errors thrown in reducer May 14, 2019

@@ -228,7 +228,9 @@ export class AppProxy {
// (because of the `Promise.resolve`).
const wrappedReducer = (state, event) =>
from(
Promise.resolve(reducer(state, event))
Promise.resolve(reducer(state, event)).catch(e => console.error(
`Error from app reducer:`, e)

This comment has been minimized.

Copy link
@sohkai

sohkai May 15, 2019

Member

I'm wondering what the correct behaviour for us should be. Should we re-throw the error, continue like nothing happened (as we do here, albeit this is dangerously returning undefined for the new state), or try to give the error back to the frontend.

I'd recommend at least rethrowing the error for now, so the observable stream stops where it is, but we should also think about having a mechanism for frontends to detect their reducer has failed.

This comment has been minimized.

Copy link
@2color

2color May 17, 2019

Author Contributor

Good point. I didn't think that far as I was a little averse to API changes and wanted to get logging before improving the mechanism. Nevertheless, this is worth looking deeper into.

I'll do some more testing and report back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.