Global error management

[pieroxy]

Description

I introduced an object called errorManager in mithril. Its goal is to be a centralized place to handle errors and logging.

Motivation and Context

Note: This is not supposed to be merged "as-is", but I am looking for feedback as to whether this type of change could be merged if I push it all the way (and also whether I did it the "right" way or not).

This change is here to help people that write apps with mithril handle errors (such as exceptions) in their code. This has two main goals:

• Monitoring: If you have an app that is deployed in the wild, keeping track of the errors on the client side is a must have. console.log cannot handle that. You need to call back somewhere to log all (or a sample of) errors happening. With this change the user can plug its own callbacks.
• Recovery: When one small component in your app throws an exception in its view component, currently, the app is dead and will not recover. I need a way to display that something is wrong and let the user try something else on another page.

Remains to be done (in my view):

• Propagate the usage to other places where console.log/warn/throw is used
• Wrap other component methods than view (oninit, oncreate, ...)
• define one more component method: onerror. Default behavior should use this method if it exists.
• Write documentation

How Has This Been Tested?

• manually for now

https://pieroxy.net/test-mithril/

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation change

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.
• I have updated docs/changelog.md

[webketje]

++ for some kind of encapsulation of the console.log|debug|info|warn statements.
Do think this kind of intermixes error-handling with debugging (which are two distinct things IMO).
I had a look at the demo page, and reminds me the problem with centralized error management is the stack trace: the first line of the stack trace should lead me to the erroring component code, not the centralized method (and preferably include the "tree of components").

I like the onerror suggestion, however this will clash with native onerror-able elements like img or iframe which expect an event argument

Maybe it's an idea to take some queues from how React handled nested component errors (error boundaries) and document a pattern that creates a "virtual" mithril element and wraps an existing component tree

[pieroxy]

+1 for the log/error management being separate concerns. That said, clients can register two events: "log" and "fail". Come to think of it, "fail" is a pretty bad name. Maybe "error" would be better.

Would you prefer having two different objects entirely dedicated? One for logging and one for error management?

As far as stack traces are concerned, if you throw new Error() you will have the proper stack trace, my code doesn't have anything that will mess up that part of exceptions (at least it shouldn't).

onerror will indeed clash with other onerror in HTML elements. Good catch. Maybe onfailure or oncomponenterror or simply onError ... Thinking out loud here.

I'll have a look at your React link, thanks

[dead-claudia]

@pieroxy I have some questions around behavior:

1. What happens if, say, a DOM element setter or setAttribute method throws an error? (Example: you mistakenly passed a symbol instead of a string, and the implicit string coercion fails.)
2. What happens to parent components when a descendant throws?
3. What happens to child components when an ancestor throws?
4. When a component throws and its error is caught, what's the behavior for that component afterwards?
5. If an event listener throws, what happens?
6. What happens if appendChild/insertBefore fails (due to a custom element reaction callback throwing)?

[pieroxy]

Hello!
First, here is a thought about what I've already done: Now that I think a bit more about it, I feel my bit of implementation should not allow listeners to return a value to be displayed. There will be two ways to handle a failure: adding a global listener or define a onfailure method as another possible lifecycle method on components.

1. onfailure lifecycle method on components duties will be to make the UI as usable as possible given the circumstances, to give a chance to the user to get out of the faulty screen and continue their journey on other pages.
2. listeners duties will be to have a global management of errors. Log them on the server, in the console, etc... You will be able to register to unhandled errors or just all errors

In this new light, my answers:

1. I didn't think of that one when designing the errorManager. I think it's very similar to n.6. I have had that happen when I mistakenly do m('some string with spaces') for example. Or returning an array of vnodes having keys where more than two keys share the same value. It's about handling crap that view methods can return and make mithril blow up when trying to apply it to the DOM after the view method is called. Ideally we should act as if the view method for the responsible component threw. I haven't had a close look but it should be as simple as climbing the node tree. (or letting the exception bubble up) ? I need to look a bit deeper here.
2. When a component throws, the errorManager will look for a onfailure in the component. If not defined, the exception will bubble up the chain until a component is found with an onfailure. If none are found, the listeners will be called to handled what is now an unhandled failure. Much like ErrorBoundaries in React or how an exception is bubbling up a callstack. So what happen to the parent depends on whether the failure was handled by one of its sub component or not.
3. When an ancestor throws, none of its descendants will be called. For view it is more obvious since we don't know its descendants (which are supposed to be deduced by the return value of view). More precisely, the descendants will be replaced by whatever onfailure returns.
4. I think it depends on where the exception was thrown. In oninit or oncreate, I'd be tempted to flag the component as erroneous and forget about it. For onupdate, view, etc we can try on subsequent redraws and maybe flag the component after a fixed number of contiguous failed redraws. Or flag it like in oninit. For onbeforeremove and onremove, just remove the component. The return value of onfailure will be ignored.
5. For errors thrown in onfailure, keep bubbling up the new exception. For exceptions in listeners, well, apart from a log in the console, I don't know what else to do for now.
6. See n.1 unless I misunderstood one of them.

Keep in mind this is a proposal open for discussion :-) I've been using mithril for all my web apps for the last 6 years, and I've been bitten a few times by my own bugs being silently blowing up on phones and browsers of my users, hence my will to handle errors correctly. But I'm still discovering the internals of mithril so I'm tiptoeing around for now.

Thanks for your interest

[JAForbes]

I personally think we shouldn't have an error manager. But curious to hear what others think.

However, I am in favour of mithril supporting dependency injection a little better, that way you could inject your own console/window etc. I wouldn't want that in the main m API, but a support import path that allows you to initialize the main API with a different environment would let you achieve a similar thing to this in userland but also support easier testing and server side usage.

E.g.

import { M } from 'mithril'

const m = M({ console: myConsole, document: myDocument, ...etc })

As for component error behaviour, I think @porsager's work with Sin is great. It's like React's error boundaries without the ceremony. I think we should probably take inspiration there but maybe in a more mithril way, e.g. return an onerror lifecycle that resolves to a view and by default it throws and bubbles up.

[dead-claudia]

Closed due to the next branch being deleted. Thanks to https://github.com/orgs/community/discussions/139697, I can't resurrect this PR, so it'd have to be re-created against main.

Sorry, forgot to post this like I did all the others.