cerebral: RFC: Improve Error Handling

One of the main goals of Cerebral is to understand the “flow” of your application. Signals give us a nice high level view of our app logic, but often times error handling clutters and adds noise to our otherwise beautiful signals.

A key design goal for error handling should be, in my opinion, that you only need to explicitly handle errors where and when your program requires it. Otherwise, error handling actually detracts from the readability of your program, cluttering it with low level book keeping instead of expressing your intent (which is what Cerebral is all about).

The Problem

Take this (Cerebral 1) signal for example:

module.addSignals({
  mounted: [
    setupDeviceSettings,
    setAuthHeaders,
    fetchCurrentUser, {
      success: [
        copy('input:currentUser', 'state:currentUser'),
        copy('state:currentUser.current_account_id', 'state:navbar.newPost.account_id'),
        fetchPosts, {
          success: [
            copy('input:posts', 'state:posts'),
            when('flags.showCommentsButton'), {
              true: [
                fetchComments, {
                  success: [copy('input:comments', 'state:comments')],
                  error: [(e) => { console.log('comments error', JSON.stringify(e)); }]
                }
              ],
              false: [/*noop*/]
            }
          ],
          error: [(e) => { console.log('fetch posts error', JSON.stringify(e)); }]
        },
      ],
      error: [(e) => { console.log('fetchCurrentUser error', JSON.stringify(e)); }]
    }
  ]
})

This signal is really just about fetching some related resources from some APIs. We need to get the: currentUser, their posts and finally the comments for their posts. We can’t render the first view until we have that data.

Notice how repetitive the error handling is above and the “pyramid of doom” with indentation creeping to the right of the screen. In this case, the developer is forced to define the error path for each API call and provide an explicit error handling action. But, really, all we care about in this instance is: 1) did everything work? or 2) did something prevent us from getting this data?

The Solution

As we talked about in the hangout, I’d like to start a discussion around a better error handling solution for Cerebral. What if we could re-write the signal above like (please forgive this hodgepodge of Cerebral 1 and 2 syntax 😛) :

module.addSignals({
  mounted: sequence([
    setupDeviceSettings,
    setAuthHeaders,
    fetchCurrentUser, {
      success: [
        copy('input:currentUser', 'state:currentUser'),
        copy('state:currentUser.current_account_id', 'state:navbar.newPost.account_id'),
        fetchPosts, {
          success: [
            copy('input:posts', 'state:posts'),
            when('flags.showCommentsButton'), {
              true: [
                fetchComments, {
                  success: [copy('input:comments', 'state:comments')]
                }
              ],
              false: [/*noop*/]
            }
          ]
        }
      ]
    }
  ], {
    error: [flash('Error fetching data')]
  })
})

The idea is to provide a named path at the top level that can be invoked if any of the nested paths throw an error. Conceptually, a nested error will “bubble up” to the top of the signal chain until it is handled. In the event that you want more specific error handling, you simply define error paths like in the first example. The key thing is that now you are not forced to repeat yourself with error handling when your application doesn’t require you to. Already, with just this one change, I think the logical flow is clearer for “the next developer”. You can more easily scan the signal without mentally removing the error paths.

It’s always a difficult balance between being low level + explicit vs concise/abstract. In this case, however, I think handling errors more concisely would benefit from a higher level API and convention.

Anyway, that’s all I have for now. Everyone please weigh in with ideas, excitement, or objections. 😄

About this issue

Original URL
State: closed
Created 7 years ago
Comments: 70 (61 by maintainers)

Most upvoted comments

hah, awesome… it worked on first try… implementation driven development 😄 Implement first, tes after!

christianalfoni on Mar 21, 2017

Hehe, yeah, abort is implicit… like, by reading the code you never know which one can cause an abort to happen. That said, it cleans up so well for these types of scenarios 😃

Ah, man, just got very happy. The “merge execution” (client/server) logic is perfect for handling ABORT 😃 Cause it is generic. It basically is a signal that has an “executedBy” prop. Meaning that an abort can fire off a new function tree execution, only it attaches the “executedBy” prop. It will automatically get inlined on the exact action that caused it… do not have to do anything 😃 Only thing is identifying it is an ABORT execution and style it.

Woop woop 🎉

christianalfoni on Mar 21, 2017

@maxfrigge Yeah, lets look more into that using the chatbot as example. Would love to look more deeply into how you solve that 😃 Maybe demo on next hangout?

Yeah, pyramid of doom, should be explained how this new thing solves that 😃 Typically you would:

[
  doThis, {
    success: [
      doThat, {
        success: [],
        error: []
      }
    ],
    error: []
  }
]

Especially on server side you typically do not want to “handle the errors”. You just want to abort the whole thing when it occurs. So first iteration you might think our new approach allows us to do:

sequence([
  doThis, {
    success: [
      doThat, {
        success: []
      }
    ]
  }
], [

])

But actually it resolves to this:

sequence([
  doThis,
  doThat
], [

])

The reason is that you no longer have to explicitly state “the happy path”, the happy path is… “just continue” 😃

christianalfoni on Mar 21, 2017

@maxfrigge I was one of the main proponents of default paths but I agree with Christian: it looks very nice and powerful in theory but it is adding quite some complexity for the “next coder” depending on how it is used so we are now focusing on errors or how we now call them exceptions.

This “smaller goal” has the advantage of not messing the readability too much.

@christianalfoni I would not bubble up. An error path belongs to the sequence where it is declared. I see “sequence” as a black box with it’s execution and exception path.

For me an error is either caught right next to where it happens (in the sequence) or at the top-most level (to avoid production app crash and show some “ouups” error).

gaspard on Mar 21, 2017

@gaspard I think one important ingredient you add to this example is:

{
toast: [showAToast],
error: [handleError, abort] // <- ABORT
}

The really good thing here is that we do not just abort, we actually compose in “How to handle errors”. Maybe sometimes we just want some default behaviour but let the signal continue or we actually want to abort current execution. That is a 👍

What is a bit risky here is that we do allow a very implicit approach to paths. There is nothing in the code that indicates this “compiling”, and you would also in the debugger risk having quite a few paths that is never used by actions. It might actually get super messy.

So what I think we need to question is: “Do we really need default paths? Or is this really about handling that something went wrong in a generic way?”. And by “something went wrong” I mean that there is no reason to continue signal execution.

I think what we really want is the second here. We want to “jump out” of an executing signal and alternatively do something related to that because there is no need to continue execution. Having default paths is an interesting idea, but I am afraid it is a “lazy” thing. It saves some code, but it reduces the explicitness of the signal as the paths will be hidden.

So here is another suggestion:

sequence([
  getThis, // Attaches some payload to props
  getThat, // Attaches some other payload to props
  thenGetThis, // And another
  computeItAll // Here we handle it all
], [
  showToast('baaad stuff')
])

With this solution we just add an additional indication that “when this signal aborts for any reason, run this sequence”. It allows us to have a pattern where the actions have no paths they go down, you just extract stuff and put it on props until you are ready to handle it. It is basically the same as a bunch of thens and a catch at the end. It is also similar to observables that has two initial callbacks. One for success and one for error. We are about chains, so we have one for everything okay and one for bailout.

Now… this causes an implementation problem, because there is no such thing as “abort and go down this path”, there is neither any existing “visual hook” for this in the debugger. That said, an abort does give information about where it happened, meaning that we could indicate an abort and what happened specifically in the debugger. Just needs to be implemented.

then + catch and two observable callbacks are proven concepts. I am afraid default paths will cause us unknown problems and some concerns are already raised. Overloading sequences with paths, making mess in debugger, and making sequences become completely implicit, making it harder to understand how they run by looking at the code.

This was a lot of text, sorry, but I hope it makes sense 😃

christianalfoni on Mar 20, 2017

@bfitch I would also like to add that these “error”, “abort” paths have nothing special about them.

There could also be things like this:

sequence('A Sequence', [
  ActionA, { error: [handleErrorInActionA] },
  ActionB,
  ActionC
], {
toast: [showAToast] // this chain is sometimes executed by any of the actions
error: [handleError, abort]
})

This above is “compiled” as:

sequence('A Sequence', [
  ActionA, { error: [handleErrorInActionA], toast: [showAToast] },
  ActionB, { error: [handleError, abort], toast: [showAToast] }
  ActionC, { error: [handleError, abort], toast: [showAToast] }
])

All the default paths are added or merged after each action. If ActionC does not return a path, it just ignores the braces (see previous comment)…

gaspard on Mar 14, 2017

Okay, thinking more. I think it will be a mistake catching errors:

There was a huge problem with Promises when they arrived cause they would eat up errors. Browsers finally implemented “uncaught promise rejection” and things got better, but like in Node it is a complete mess, no strack trace on those (at least to my knowledge). Anyways, the problem is that now we eat up all errors that are not explicitly handled or rethrown
Errors are not serializable… we want to give a payload to the catch sequence. The error itself can not be the payload, cause that would give problems with existing payload we want to pass. We could hardcode it to be on the error property, but still… serialized errors will just be “{}”
Errors can not pass object of information, just a string. But we want to bail out of execution and pass information to the catch sequence, just as if you returned any payload from a function
Even errors related to React would be eaten up, because a signal synchronously triggers a flush to the UI running code there… any faulty code could end up in the catch sequence, being eaten up

The current approach solves all of this. We do not eat errors you want to know about, we do not have to make some opinion and spray fairy dust on making errors serializable and now we can pass any payload to the catch sequence.

christianalfoni on Mar 22, 2017

Okay, so been lots of discussions today and I have been thinking about function-tree low level nature. With Cerebral we have many ways to handle errors, but function-tree should probably not innovate too much 😉

What I specifically mean is that function-tree should probably work like Promises and Observables. You attach “catch” sequences and those are run when an error is thrown. And if there is no catch sequence, it bubbles up to find one. If none is found, an error event is emitted.

I agree with @gaspard that native JS errors are annoying to get into your business logic “catches”, but I think it is even more important to have familiar behaviour. We can just say: function-tree works like promises and observables. It has catch logic on throwing that bubbles up.

So in code you would do this:

sequence([
  function someFunc () {
    throw 'something'
  }
], [
  // current payload with merged in {error: 'something'}
])

Same with promise:

sequence([
  function someFunc () {
    return new Promise((resolve, reject) => reject('something'))
  }
], [
  // current payload with merged in {error: 'something'}
])

That would mean there is no special “abort” in function-tree. The debugger would rather highlight the action with RED as normal, but instead of showing code the failed, it would show the execution of the catch sequence.

Been twirling around in my head and I think at this level we need to be more familiar. We can rather do stuff in Cerebral to make error handling more approachable. Has already been suggestions on allowing modules to handle errors, we could expose error operators like:

sequence([

], whenError({
  'SyntaxError': [],
  'MyAppError': []
})

Where it would otherwise throw the error further “up” 😃

christianalfoni on Mar 22, 2017

@bfitch so you can still build around the happy path and instead returning error path you abort… so less error paths in the signals?

maxfrigge on Mar 21, 2017

@christianalfoni when using function-tree for signals in cerebral there might not be a good abort. But when using function-tree in a different environment there may be cases where you want to end early. I have such a case in my chat bot with function-tree project … but yes this might not be the intended use … just saying 😃

@gaspard well said! so the takeaway is… keep the pyramid of doom in favour of better visibility - is that about right?

maxfrigge on Mar 21, 2017

@gaspard Great, we need to do that first anyways. If bubbling shows itself useful it can be added later 😃

christianalfoni on Mar 21, 2017

@fopsdev It would be the same 😃

parallel([

], [

])

When an exception occurs it will just stop… the result of “the other actions” will still be resolved though, as you can not cancel a promise. But the signal will not continue to execute 😃

christianalfoni on Mar 21, 2017

In Python we use the EAFTP (Easier to ask for forgiveness than permission) coding style. So e.g. for check if keys or attributes are valid we use try/except blocks instead of if statements. You can just throw somewhere an exception and catch it later in the code for handling.

So this is a great and powerful pattern and even when Javascript is not so optimized for this as Python it still makes sense.

henri-hulski on Mar 21, 2017

As an extra step, I would consider adding a ‘continue’ thing that would run the next action:

export const  mounted = sequence([
  setupDeviceSettings,
  setAuthHeaders,
  fetchCurrentUser, 
  set(props`currentUser`, state`currentUser`),
  set(state`currentUser.current_account_id`, state`navbar.newPost.account_id`),
  fetchPosts,
  set(props`posts`, state`posts`),
  when(state`flags.showCommentsButton`), {
    true: [
      fetchComments,
      set(props`comments`, state`comments`)
    ],
    false: [/*noop*/]
  }
], {
  success: sequence.continue,
  error: [flash('Error fetching data')]
})

gaspard on Mar 13, 2017