Error handling in view

[andraaspar]

It seems like Mithril does not handle errors thrown in the view function.

My personal experience is that if an error is thrown inside the view, the UI does not get properly updated. Not only the component fails to be updated, but the entire app as well.

Context

In this example, an error is thrown inside ErrorComp.view. The error prevents UI update in the entire app, hence button label is not updated.

var data = {
  flag: true
}

var ErrorComp = {
  view: function () {
    console.log(data.flag)
    return (data.flag ?
      `No error.`
      :
      undefined.error
    )
  }
}

var App = {
  view: function () {
    return [
      m(ErrorComp),
      m('div', 'Click the button below.'),
      m('button', { onclick: () => data.flag = !data.flag },
        'Toggle flag: ' + data.flag
      )
    ]
  }
}

m.mount(document.getElementById('app'), App)

Fiddle here.

Steps

1. Click the button.

Expected

ErrorComp not to render, but the rest of the application to update (button label to show false).

Actual

The UI does not update.

Suggestion

Mithril could catch the error and render an empty string where the component should be (and log the error). Maybe an errorView method could be supported (if defined).

It is all too easy to get an error in a component. For example if part of a data path is undefined, as loaded from AJAX. I think keeping the rest of the UI up to date would result in a more robust application. One that I as a developer can trust will work in the wild.

I know I can work around this, and I do. But I think Mithril could do better, and be more friendly to devs.

[dead-claudia]

I agree it'd be nice to have it clear the view on own or child error (React changed to do this recently), and to have some sort of error handling function. In the past, it's been suggested to just have m.render/m.mount/m.route return a stream or similar, but something like React's error boundaries and/or this is probably the best route.

Note that what React has is basically a glorified "catch", and they idiomatically prefer updating the state to request a redraw, which basically causes the view to be re-rendered. That specific solution is bad design IMHO - should you really by default be re-entering a method that just threw?

[dead-claudia]

Also, I'll drop an obligatory note: AJAX errors aren't synchronous, so there's nothing we can do to handle those kinds of errors.

[andraaspar]

@isiahmeadows I meant data from an external source may have undefined properties. Not AJAX errors.

[orbitbot]

I'm not really sure that if the best solution would be to update 98% of views or not, there may be an argument for easier detection of something going seriously wrong if everything goes seriously wrong.

The surprising part of the sample fiddle was IMO that there was no error logged, rethrowing at the end of the redraw cycle would probably be ideal.

[dead-claudia]

@orbitbot Thought I'd clarify a few things:

• The errorView proposed would effectively act as a catch, so errors would only be re-propagated if that itself throws.
• If no hook exists or a hook exists that throws when we call it, we'd remove that subtree.
• Nothing here so far states whether we'd call onbeforeremove/onremove on error, or if we do, how to handle errors thrown from those methods.

[andraaspar]

@isiahmeadows Calling lifecycle methods would be awesome! A lot better than I can do today!

[thomp]

Regardless of whether a more sophisticated catch equivalent is developed or not, silently handling the error ("The surprising part of the sample fiddle was IMO that there was no error logged") hardly seems desirable behavior. Perhaps 'Expected' behavior should be extended to explicitly include both points -- (1) rest of the app updates and (2) the error logged or otherwise made visible to the calling function?

[dead-claudia]

@pygy @tivac WDYT about this feature request (and maybe my proposal)?

[mpfau]

Just lost an hour because errors in view methods are currently swallowed by mithril: https://github.com/MithrilJS/mithril.js/blob/bdb04e1772467ee8f7de53c5f3721861c46c425c/mithril.js#L1046

Logging to the console is a must have from my point of view.

[pygy]

@mpfau Thanks for the report, my bad... Errors should be logged with console.error() at that point (this was meant to address this issue, but with coarser granularity (mount point rather than component)).

[pygy]

@mpfau I just pushed a temp fix for this for those who use the next branch. IE10+ though (console is undefined in IE9 unless the dev tools are open). Do we still care about IE9?

[orbitbot]

Wouldn't that throw in IE9, or are console messages swallowed?

[pygy]

@orbitbot yes it would hence my remark. We could wrap the console.error call in a try/catch block. User facing behavior would be consistent. (And the devs would get the messages when they open the console).

[orbitbot]

Details, but AFAIK you can also just check for the existence of window.console before logging to avoid that much more complex code.

[pygy]

@orbitbot I put a typeof console !== 'undefined', the fewer window references in the code base, the better.

[nandoflorestan]

Differently from some here, I do not want any error catching at all. In my application, I would prefer the entire page to crash if anything goes wrong. In fact, I consider half-working pages to be a very bad and confusing thing for users and for bug squashing in general. PHP practices notwithstanding.

Anyway, as much as I love mithril, it is creating a debugging nightmare by swallowing any exceptions thrown in my components (in the constructor or in the view method). Putting something on the console is the very least that mithril must do.

This is the most urgent fix that mithtril needs IMO. Someone said using "next" has this problem mitigated. Where can I read about what the consequences of using "next" are?

Although this is so urgent, it has lingered here for months. Could someone do something?

[pygy]

@nandoflorestan could you post a repro? I can't have it swallow errors in either v1.1.6 or the next branch.

[dead-claudia]

@nandoflorestan Also, could you file a new issue WRT that? This is about allowing optional opt-in graceful handling of errors. (Currently, if any errors are being swallowed within the framework, that's a bug.)

[michaek]

Hi, folks. I'd like to report a subtler wrinkle to this issue: under v1.1.6 if the first render of a component throws an error, it is caught and logged, but it leaves the vnode in a state where it will throw confusing errors about 'onbeforeupdate' in subsequent renders, with most likely serious impact on app behavior. See https://jsfiddle.net/v2dL1cLw/2/ This does not appear to be an issue in next, but I'm mentioning it (other than as a historical curiosity) because 1.1.6 is still the most current stable release.

[michaek]

Unless I'm mistaken, when a mounted 1.1.6 app contains any component that throws an error from its view on the first render, that app is bricked - every subsequent redraw will stop on an exception from mithril's internals. This isn't necessarily a state that a developer can recover from, as corrupted data could be unexpectedly provided from an external source (server request, localstorage, etc) at runtime. I can't imagine this is the intended behavior.

(Note: I was mistaken in my comment above when I said the error was caught and logged. It's not.)

As the original reporter noted, "I know I can work around this, and I do", but the workaround is pretty extreme: every component in your app needs to catch and handle errors within its view, never allowing them to reach mithril's internals, or the app will permanently stop redrawing. This seems like a very serious issue to me, perhaps the gravity got lost in some of the side-issues along the way, but it almost certainly deserves a fast fix on 1.1.x rather than waiting for 2.0.

[michaek]

@isiahmeadows I think it's possible the labels on this issue aren't correct. The original reporter was only suggesting a feature change (errorView) in passing - this issue is primarily describing a bug.

[michaek]

I think this issue may actually have the same underlying cause as what @barneycarroll reported in https://github.com/MithrilJS/mithril.js/issues/2067 . While that issue is marked as closed (code complete) no fix has been released yet...

[dead-claudia]

@michaek

No, the original reporter was asking for Mithril to not blindly swallow exceptions, but to actually handle them meaningfully, a feature request.

var ErrorComp = {
    view: function () {
        console.log(data.flag)
        throw new Error("something went wrong")
    }
}

// Let's try to render it
m.render(document.getElementById("app"), [
    m('div', 'Some text')
    m(ErrorComp),
    m('div', 'Some text')
])

<!-- The original element -->
<div id="app">
    <button onclick="alert('Hello!')">Click me</button>
</div>

<!-- What's currently rendered, ignoring comments/whitespace -->
<div id="app">
    <!-- Element cleared, but `ErrorComp`'s view caused rendering to fail -->
</div>

<!-- What OP wants, ignoring comments/whitespace -->
<div id="app">
    <div>Some text</div>
    <!-- ErrorComp's view failed to return a vnode tree, so it's ignored -->
    <div>Some text</div>
</div>

It's distinctly different from #2067, which is about an (admittedly difficult and obscure) bug about vnode._state getting erroneously reset when it shouldn't be.

[michaek]

@isiahmeadows I still think you're mistaken about the original reporter. Later in the conversation, there's discussion of swallowing errors, but that's not what this issue was originally about. Specifically, the reporter says "the error prevents UI update in the entire app" - which isn't reporting that they can't handle the error themselves, but that it kills redraw.

It's as a result of digging into the report and the provided fiddle that I observed the bug from #2067 , which seems to be caused by exiting initComponent early due to an uncaught exception.

[michaek]

I'm realizing in retrospect that the original fiddle would be exiting early from updateComponent (since its initial state is error-free) not initCompoent. Since details matter in this, it's annoying to mix them up, sorry!

I'm not sure whether that would trigger the same conditions, but I think I was able to reproduce the _state.onbeforeupdate issue from both paths, the key addition to the test case being an interval that would subsequently call m.redraw against a component that was incompletely set up.

[michaek]

@barneycarroll You asked on #2241 for an example that would necessitate the change, but it seems you've already noticed the fiddle in https://github.com/MithrilJS/mithril.js/issues/1937#issuecomment-427992010. I agree that "if it breaks, it breaks" is a good general behavior, but the problem this exposes is that an unexpected error in rendering a view (that is, any view author didn't handle all possible exceptions) will cause all subsequent redraws to fail - an exception in any component can brick the app and require a reload. That's a big deal in a browser, but it's much more serious in a webview in a native app, such as under Cordova. I understand that a new major version is on its way, but I think it should probably be considered unacceptable for an issue like this to remain in the stable release.

[dead-claudia]

@barneycarroll @michaek I'll give a concrete proposal here:

• I'll add a hook called oncatch to define an error boundary. When omitted, the default behavior is to just rethrow the error.
• On component error, the vnode's/component's oncatch is called. If this throws (or doesn't exist), we clear the component/vnode and propagate the error to the parent, which then tries the same steps.
• When removing components due to errors, if they didn't themselves error, we call onremove as normal (so they can release resources), but we skip onbeforeremove and related animations.
• In the special case of a component with both an oncatch attr and an oncatch component hook, the attr is called second, and only if the component rethrows. (Related: #2270)
• If an error propagates to the root, the root is cleared and we rethrow the error.
• In the special cases of onbeforeremove and remove:
  • If an error occurs in onbeforeremove, we call oncatch instead of onremove, but if it rethrows, the error is just logged.
  • If an error occurs in onremove, we still call oncatch, but if it rethrows, the error is just logged.
  • The reasoning for this is that we're removing the element either way, so the invalid state isn't going to be used again except maybe in the error handler.
  • In onbeforeremove and onremove, if a component's hook errors, it doesn't prevent attrs hooks from being called and vice versa, if an attr hook errors, it doesn't prevent the component's hooks from being called.
• Errors in event handlers trigger this behavior, too, so if something throws in an onclick, you can handle it like any lifecycle error.

@MithrilJS/collaborators I'd like to short-list this for v2. WDYT?

[barneycarroll]

I noticed the fiddle, but it doesn't really illustrate any wider problem other than 'runtime exceptions crash the runtime'. I guess I'm angling for a rationale as to why this shouldn't be the case, and why and how Mithril should work around that.

That's a big deal in a browser, but it's much more serious in a webview in a native app, such as under Cordova.

If it's a case that you half expect your code to throw errors by design, and thus have the application be fault-tolerant in production, it's kind of up to you to determine where and why that is and what user feedback you'd prefer.

If it's a developer experience issue - it's annoying to have to restart the Cordova VM to see if your fix works? - I think if you can get a stack trace that shows you where your exception took place, that's not such a bad thing: arguably a framework that captures your application errors and still allows the lifecycle to partially resolve around it, is detrimental to the task of identifying the error and fixing the application logic that causes it. My experience working with React, where mapping cause and effect to lines of your own application code is nightmarish - everything is so heavily abstracted that any sign of a problem is signalled in indistinguishably generic logs with a stack trace that steps back dozens of frames into library core. The end result on development culture is that while parts of the application can continue to run despite some buggy code which might be none of your business - that errors become harder to fix, identify, many end up deemed tolerable, and the application becomes increasingly buggy and hard to reason about as time goes one.

Perhaps a custom development harness would help? I had a bash at something like that here (flems)

Edit: catered for resolution in thrown onbeforeremoves, reinstated autorun app code

[dead-claudia]

@barneycarroll

If it's a case that you half expect your code to throw errors by design, and thus have the application be fault-tolerant in production, it's kind of up to you to determine where and why that is and what user feedback you'd prefer.

It'd be super helpful if Mithril could provide the required primitives for fault tolerance, though, and that's basically what people here are asking for: built-in fault tolerance for rendering + basic primitives to leverage that in their applications.

And yes, mitigating and logging production crashes (so you can properly handle them and maybe even restart the component) are the main selling point of these kinds of hooks. It's what powered the zones proposal (used/pushed by Angular), it's why domains are so popular in Node backend servers, and it's why React added componentDidCatch.

My proposal was strongly inspired by React's, but there are several minor differences like calling onremove for non-erroring components that need removed.

[michaek]

@barneycarroll I'm concerned with user-facing exceptions, not the developer experience. Your flem is a good example of the code you'd need to have confidence that you're not going to brick on an unexpected error, but it seems strange to expect something like that to be a userland implementation. As soon as anyone (new dev on the team, third party component) manages to use a component with a stock m, you're out of the safety zone, and any exception is unrecoverable.

@isiahmeadows I agree that mechanisms for fault tolerance would be excellent, and I think that's what some of the discussion on this issue is about. I'm just trying to scope this issue to safety in production, for 1.1.x (or whatever the current stable release is) - the problem isn't that the error can't be handled by application code, it's that an error thrown in a view leaves mithril's internal vdom state broken and throwing spurious errors on subsequent redraws, as in #2067 . A band-aid fix to 1.1.x would seem to be an acceptable solution to this facet of the issue. I didn't manage to dig down to what was actually breaking in the vdom, so my PR was not even a band-aid, just a hack.

I was not able to reproduce this issue in 1.2, so my assumption is that as soon as a new release supersedes 1.1.x as the stable release, this issue is probably dead.