Implement basic error tolerance (WIP)

[dead-claudia]

Description

I deferred all hook errors to the end (after we render) and I just rethrow the last one we catch. This is so if an error occurs, things still get logged rather than breaking.

Note that if onbeforeupdate throws, I don't update the children. This is the only case where I don't treat throwing identically to undefined, but if we don't know if we could update without error, we should err to the side of caution and just not try.

Motivation and Context

Fixes #1937

How Has This Been Tested?

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/change-log.md

[dead-claudia]

I've got this milestoned for v2, but it's not an absolute blocker. Anything here is technically non-breaking and falls in the realm of a bug fix, so we could ship it in v2.0.1 if necessary.

[pygy]

Using the closure for state would make m.render non-reentrant.

Not sure it is documented, but I've relied on that property in the past to implement React Portal-like components in Mithril.

I'll look at the meat of the patch later this evening.

[dead-claudia]

You can save variables and restore them in a try...finally to preserve reentrancy, and that was the implication here. It's pretty straightforward, and it's semantically equivalent to just using locals, just while taking up considerably less stack space and simplifying calls than duplicating its reference every single time we descend into children. I've already been doing it with the third parameter redraw function (in part to reduce the patch size) and have considering doing similar for other rarely-changing values like the namespace URI we use to create elements with, to further reduce the cost of recursion. And yes, while this is normally insignificant, with real-world trees and how much we recursively jump around, even insignificant stuff like this gets magnified tremendously and so I suspect I could get a few perf wins out of it as well. (In my experience with this framework, it doesn't take much to slow everything down by a factor of two.)

On Sat, Dec 14, 2019 at 14:00 Pierre-Yves Gérardy ***@***.***> wrote: Using the closure for state would make m.render non-reentrant. Not sure it is documented, but I've relied on that property in the past to implement React Portal-like components in Mithril. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2287?email_source=notifications&email_token=ABCGWBHEO2V47ANMTMA36DTQYUUMLA5CNFSM4GCMXTXKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEG4I4UI#issuecomment-565743185>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABCGWBGKQ34QZCKFZ4WTTG3QYUUMLANCNFSM4GCMXTXA> .

-- ----- Isiah Meadows contact@isiahmeadows.com www.isiahmeadows.com

[pygy]

Indeed, I had missed that bit. Good point, and nice implementation. +1 for doubling down with the NS parameter.

[pygy]

The previous discussions went over my head, actually, I couldn't see how to do something useful to tackle the issue. Now I see :-)

I like the approach in general, but I have reservations for the method used to report the errors.

Also I wonder if it is complete: is the lack of hookThrown detection in onremove() deliberate?

Tests will also be needed (I can help).

[pygy]

Probably the last remnant of the vnode cache... Good riddance I guess :-)

[pygy]

Shouldn't we rather use console.error(nextHookErrors[i]) so that the stack is available and interactive in the console, then throw "Errors occurred during rendering"?

[dead-claudia]

@pygy I'll file a new PR with a version that actually works with the current system - it's an utter mess and the push/restore bits would need to integrate with the redraw bits as well. I also have to errors during removal (both sync and async) better - they should be swallowed, but logged, and the logging part should also be a setting you can set. (I'll want to update the third parameter to m.redraw to accept either a {redraw, onerror} object or a redraw function.) Edit: I decided against this. See my new comment in #1937.

Separately, and I'll file this in the main issue, I found a way to add error handling without breaking things, but it'll require a fragment-like control vnode, something that'll be a bit new (providing a small preview of what the redesign will end up looking like). Edit: Link to proposal.