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

Error handling in view #1937

Open
andraaspar opened this issue Aug 14, 2017 · 34 comments
Open

Error handling in view #1937

andraaspar opened this issue Aug 14, 2017 · 34 comments
Labels
Area: Core For anything dealing with Mithril core itself Type: Bug For bugs and any other unexpected breakage
Milestone

Comments

@andraaspar
Copy link
Contributor

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
Copy link
Member

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
Copy link
Member

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.

@dead-claudia dead-claudia added Type: Breaking Change For any feature request or suggestion that could reasonably break existing code Type: Enhancement For any feature request or suggestion that isn't a bug fix labels Aug 15, 2017
@andraaspar
Copy link
Contributor Author

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

@orbitbot
Copy link
Member

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
Copy link
Member

@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
Copy link
Contributor Author

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

@thomp
Copy link

thomp commented Aug 23, 2017

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
Copy link
Member

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

@mpfau
Copy link
Contributor

mpfau commented Sep 24, 2017

Just lost an hour because errors in view methods are currently swallowed by mithril:

for (var i = 1; i < callbacks.length; i+=2) try {callbacks[i]()} catch (e) {/*noop*/}

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

@pygy
Copy link
Member

pygy commented Sep 24, 2017

@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
Copy link
Member

pygy commented Sep 24, 2017

@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
Copy link
Member

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

@pygy
Copy link
Member

pygy commented Sep 25, 2017

@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
Copy link
Member

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

@pygy
Copy link
Member

pygy commented Sep 25, 2017

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

@nandoflorestan
Copy link

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
Copy link
Member

pygy commented Aug 9, 2018

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

@dead-claudia
Copy link
Member

@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
Copy link

michaek commented Oct 8, 2018

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
Copy link

michaek commented Oct 9, 2018

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
Copy link

michaek commented Oct 9, 2018

@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
Copy link

michaek commented Oct 9, 2018

I think this issue may actually have the same underlying cause as what @barneycarroll reported in #2067 . While that issue is marked as closed (code complete) no fix has been released yet...

@dead-claudia
Copy link
Member

dead-claudia commented Oct 9, 2018

@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
Copy link

michaek commented Oct 9, 2018

@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
Copy link

michaek commented Oct 9, 2018

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.

@dead-claudia
Copy link
Member

@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
Copy link

michaek commented Oct 29, 2018

@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.

@dead-claudia
Copy link
Member

@pygy, @barneycarroll, and I came up with a plan.

Here's the concerns I believe need addressed when it comes to fault tolerance in Mithril, in decreasing priority:

  • Mithril shouldn't break over user errors
  • Mithril shouldn't hide user errors
  • Mithril shouldn't let caught user errors affect unrelated components
  • Mithril shouldn't allow potentially invalid state to be used
  • Mithril shouldn't let resource leaks propagate
  • Mithril shouldn't prevent error recovery

The first two bullets will be the focus of this bug, but the other 4 will be discussed in #2273.

Resolving the first 2 will mean this flems should log oninit + oncreate and this flems should log oninit + oncreate + onbeforeupdate + onupdate. (Currently the first logs just oninit and the second logs oninit + oninit + oncreate.)

@dead-claudia dead-claudia added Type: Bug For bugs and any other unexpected breakage and removed Type: Breaking Change For any feature request or suggestion that could reasonably break existing code Type: Enhancement For any feature request or suggestion that isn't a bug fix labels Oct 30, 2018
@dead-claudia dead-claudia added this to the 2.0.0 milestone Oct 30, 2018
@dead-claudia dead-claudia modified the milestones: 2.0.0, post-v2, 2.1.0 Jul 24, 2019
@dead-claudia dead-claudia modified the milestones: 2.1.0, post-v2 Sep 30, 2019
@dead-claudia
Copy link
Member

Pardon for the radio silence here! It's been awfully quiet, and I've been spending more time on the job hunt and elsewhere than actually writing code for Mithril. Plus, I've had an uncooperative computer for a while, which certainly hasn't helped things.

So here's my current plan. Note that assuming this gets merged (unlikely before v2.1.0), it would be released in one atomic step in a future minor release. I'll likely run a release candidate with this first, so it gets some testing in case I miss something, as it is a complicated thing to implement.

Error tracking

Errors are tracked and captured in three cases:

  1. When initializing and invoking hooks on the current component, including async rejections from onbeforeremove
  2. When initializing and invoking hooks on child components, including async rejections from onbeforeremove
  3. When initializing and updating properties on elements

Other errors may be tracked and captured (such as some internal errors), but only these are guaranteed captured.

Errors are classified by phase. If an error occurs in either onbeforeremove (including async rejections), onremove, or while directly removing a subtree, it's considered a remove error. If it occurs at any other point, it's considered an update error.

Errors are captured along with a stack trace of components. This is used so components can know where an error originated from and optionally send that pretty-printed to wherever.

Error handling

A special vnode m.recover({onerror, onremoveerror, ...}, ...children) exists to capture errors in a subtree. It accepts a key and all the usual lifecycle methods fragments accept, but it also accepts two additional attributes unique to this:

  • onerror(e, stack) -> vnode is called with the caught update error e along with the stack trace explained above, a simple array of components, on errors caught during updating, and returns a replacement vnode tree. To propagate either the existing error or a new error, throw it from the callback.
  • onremoveerror(e, stack) -> void is called with each remove error e that propagates from the subtree, along with the usual stack trace for it. The return value for this is ignored, but errors thrown from this are propagated as expected. These are invoked asynchronously.

In both cases, the stack trace received by each callback is not further used by Mithril after you receive it, so it's safe to mutate and so on.

If you omit either attribute, it's simply propagated.

This separation is because errors that are caught by onremoveerror do not cause their child tree to be removed and are merely logged by default. Errors that would be caught by onerror unmount their tree.

The .tag for this vnode would be "!", a new tag dedicated purely to this kind of vnode. And m("!", {onerror, onremoveerror, ...}, ...children) works as per usual.

At the top level, you can just do m.render(vnode, m.recover({...}, ...)) if you wish to set a global default error handler.

Error propagation

Errors propagate from their child to their parent automatically unless the child does has its .tag set to "!" (as in, is an m.recover vnode). When an error during removal propagates outside a vnode, nothing is done. When an error during any other phase propagates outside a vnode, the resulting subtree is removed.

When errors propagate past the root, they trigger default handlers:

  • Update errors: the subtree is removed and the error is then re-thrown from the root synchronously.
  • Remove errors: the error is logged to the console using the global console.error.

Impact

This is technically non-breaking, as it codifies currently-undefined behavior.

  • The error handling previously just broke the renderer, and was a "here be dragons" area. Although this could break some apps that rely on things not being removed, state errors could have arisen regardless and it not breaking was a matter of luck, likely for similar reasons shouldNotUpdate throws an uncaught exception: vnode._state is undefined #2067 had the limited impact it did.
  • A new vnode is something tools like mithril-node-render need updated for, but this no new errors could appear until you tried using the new vnode.

Future

In the future, I'd like to also hook up event handlers similarly so errors from them get propagated from their originating component. However, this being a breaking change, that's not going to be in this and will wait until v3 before it'd land.

@michaek
Copy link

michaek commented Aug 17, 2020

@isiahmeadows Pardon me as well for my radio silence! This plan sounds great to me. I'm ambivalent about future plans for propagating errors from event handlers. 😄

@dead-claudia
Copy link
Member

@michaek The idea is that errors from event handlers still could risk invalid state occurring. And of course, this is just my proposal - it's not necessarily the final product, and I very much do want to encourage discussion on the specifics of it. 🙂

@dead-claudia dead-claudia added the Area: Core For anything dealing with Mithril core itself label Sep 2, 2024
@dead-claudia dead-claudia moved this to Planned/In Progress in Feature requests/Suggestions Sep 2, 2024
@dead-claudia dead-claudia mentioned this issue Oct 13, 2024
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Core For anything dealing with Mithril core itself Type: Bug For bugs and any other unexpected breakage
Projects
Status: Planned/In Progress
Development

Successfully merging a pull request may close this issue.

9 participants