[feat] Error handling API as discussed in #1096

[rster2002]

Started a draft for the Error Handling API that was discussed in #1096. Created this draft pull request to track progress and for feedback on the code/changes.

The proposed feature would add a onError event to components that catch errors thrown within the component and would work something like this:

<script>
import { onError } from "svelte";

var a = {};

onError(e => {
    // Handle error
});
</script>

<span>{a.b.c}</span>

Let me know what you think.

Todo

Tasks that need to be done before merging.

• Catch errors that occur during the initial component creation (contents of script tag)
• Catch errors from event listeners (for example on:click)
• Catch errors that occur in template blocks
• Catch errors that occur in computed values/blocks
• Catch errors that occur in dynamic attributes (placeholder={})
• Catch errors that occur in bindings (bind:value={})
• Catch errors that occur in async block without catch
• Catch errors that occur in if blocks
• Catch errors that occur in each blocks
  • Catch errors that occur in keyed each blocks
• Catch errors that occur within actions (use:action)
  • Catch errors that occur when action is destroyed
• Allow errors to bubble to parent component
  • Bubble to parent component when the child is in a slot of the parent
• onError should only run once with uncaught errors
• SSR
• Write tests
• Write documentation
• e0568fd#diff-da9bae4e28c441de5ba3a074e30775fe69109100b3d921ad8f2592d93cd67b7fR139

Cleanup tasks

These are not critical for the feature but are mostly code improvements.

• ad2aba8#diff-d94cf50c83c7e904480757826030849637a741d5ca6d1c194e0a6eaee7fc08f1R409
• c8736dd#diff-d94cf50c83c7e904480757826030849637a741d5ca6d1c194e0a6eaee7fc08f1R446

Before submitting the PR, please make sure you do the following

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• Prefix your PR title with [feat], [fix], [chore], or [docs].
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests

• Run the tests with npm test and lint the project with npm run lint

[rster2002]

To manually bubble a error I've implemented the following method:

<script>
import { onError } from "svelte";

onError(e => {
   // Handle error
   throw e; // Throw to next component
});

function triggerError() {
    throw new Error("Test");
}
</script>

<button on:click={triggerError}>Trigger error</button>

I've implemented it this way because it feels closest to what you would do when catching errors with a try catch block, but as the original issue suggests; bubbling could also be done by returning true.

[Crisfole]

Looks pretty fantastic. The errors look like they only happen on Ubuntu? The lint errors should be straightforward typescript issues...

I don't see any tests, but that would be pretty useful for a draft PR because it would give concrete discussion material. Tests I could see being useful:

• A super simple Error boundary component.
• Handling an error from a nested component that does not handle its own error.
• Handling an error from a nested component that bubbles its errors.
• an error with no error handling in it's component tree.
• SSR errors (looks like this should work already based on what i understand here)
• Errors from a slot in a child component with an error handler (personally i think a slot is the property of the parent which is providing the slot)
• errors from a slot in a child component with no error handler, but with an error handler on the parent.

I'm sure there are more, but wanted to get my thoughts down

[rster2002]

Thanks for your comment.

I'll look into writing tests as you correctly pointed out that it would provide good value to immediately write them. I'm not familiar with writing tests yet, so it'll probably take some time. I've placed a comment on the RFC about the slots as I'm not really sure how people would like it to function and what the trade-offs are. If you could leave your thoughts there I would appreciate that (sveltejs/rfcs#46 (comment))

In the mean time, I'll try to fix the bugs that occur after the initial commit to get the tests green again and then start working on the tests.

[rster2002]

Have been thinking a lot about this situation:

<script>
    import { onError } from "svelte";

    var a = {};
    export var error = false;

    a.b.c // Error is thrown here

    onError(e => {
        error = true;
    });
</script>

Where the error is thrown before the onError handler is registered. Would it be desired to let the handler register before any of the user code or would that transform the user code in such a way that it would cause confusion?

[rster2002]

It seems most of the failing checks are caused by the js section of the tests where the js output is directly compared to an example. Is there something I need to do to make this passing?

[bluwy]

Maybe npm run test --update per the contributing guide? If they should actually be updated.

[rster2002]

I must have glossed over that section of the guide, thanks for pointing it out. It doesn't seem to work though. Tried npm run test --update -- -g js to try and update only the JS tests to check but no luck so far.

[bluwy]

I can't really help as I'm not sure what actually doesn't work. Perhaps you can ask around in discord to get a quicker response from the maintainers.

[Crisfole]

Have been thinking a lot about this situation:

<script>
    import { onError } from "svelte";

    var a = {};
    export var error = false;

    a.b.c // Error is thrown here

    onError(e => {
        error = true;
    });
</script>

Where the error is thrown before the onError handler is registered. Would it be desired to let the handler register before any of the user code or would that transform the user code in such a way that it would cause confusion?

Personally I'd only catch stuff subsequent to that. The main point (in my mind) to the new hook is catching errors in code that happen in templates where they're currently does not exist a way of handling the errors without lots of effort. One thought i did just have:

How does this handle Promises? If a promise rejects and is unhandled does this code catch that?

[rster2002]

Good point, should probably look into the promises because it probably doesn't catch.

[Crisfole]

Honestly, I'd forgotten that components weren't Promise-based by default.

A svelte component has a complex lifecycle, simple to understand for a user which is a huge win, but complex on the back-side of things.

I'd expect the onError hook to 'hook into' all parts of the lifecycle: construction, onMount, onDestroy, tick, beforeUpdate, afterUpdate, template code, and reactive code.... Having a single hook that handles it all still makes tons of sense to me....I can answer questions on what I think is desirable behavior...but I'm not 100% clear on an implementation quite yet. I've never actually dug too deep into Svelte's compiler code.

Your case is particularly tricky because its a promise without any connection to the component after resolution or rejection, and a Promise doesn't maintain any connection to the callpoint without using a closure...In that particular case I don't think I'd expect the onError hook to get called unless the function being called altered component state or unless the promise was handled with a continuation that altered component state...maybe? I'd welcome thoughts.

[rster2002]

My initial thought after testing promises using the hook was to make it clear in the documentation that the hook only catches synchronous code and that asyncronous code should be handled by the user using .catch It's not very clean to create this disconnect between different kinds of errors, but right now without having to make a good part of the lifecycle async I think it's best to leave it this way.

A workaround might be to expose a method that can be used to attach to promises so the error can still be handled by onError and can still bubble to other components.

<script>
    import { onError, handleError } from "svelte";

    onError(e => {
        console.trace(e);
    });
    
    function rejectPromise() {
        return new Promise((res, rej) => {
            rej();
        })
            .catch(handleError); // Attach the svelte error handler
    }

    rejectPromise();
</script>

This allows the user to opt-into using the svelte error handling.

[rster2002]

Also another thought after your comment, would you expect it to catch error that are thrown by the lifecycle itself (so code outside of the part the user has control over?)

[rster2002]

How should the following work:

<script>
    import { onError } from "svelte";

    let a;
    let error = false;
    onError(() => {
        error = true;
    });

    a.b;
</script>

{#if error}
    Caught
{/if}

Should the text show or not? Because if it should there are a lot of things that need to change and it may or may-not be worth it for what you get out of it as it requires changes to how the ctx is initially created.

[Crisfole]

I would expect it to show, yes.

[rster2002]

Personally I would make the argument that it could cause confusion on what parts keep working and what parts won't work in this case. In my opinion a component could either mount completely or would not mount at all, but without anyone else weighing in this would be hard to dicide.

[benbender]

Just my 2ct regarding the last discussed topics:

• onError-handler registered after the throwing code:

I would assume it to catch the error as I would assume that it's transformed on a lower (compiler)-level. This would feel in line with onMount() and the other lifecycle-methods.

• for the rejected promise:

I basically would assume the same behaviour like if I would do something like:

try { Promise.reject("error"); } catch (e) { console.log("catched"); }

So atm, missing top-level-await, I would expect it to not catch the reject and log an error to the console. Additionally the await/then/catch-syntax in the template exists to handle those cases.

• for the "should the error be shown"-question:

Yes, it should. Execution should be resumed after the error is handled (and no new error is thrown). Otherwise it would contradict the whole purpose of such a hook imho.

[rster2002]

Concerning the last item on your list, what would you expect to happen in the following situation?

<script>
    import { onError } from "svelte";

    let a;
    let b = {};
    let error = false;
    onError(() => {
        error = true;
    });

    a.b; // Error is thrown here

    b.c = true;
</script>

{#if error}
    Caught
{/if}

{#if b.c}
    Property `c` on `b` is true
{/if}

[benbender]

Concerning the last item on your list, what would you expect to happen in the following situation?

Good point. It depends on the mental modal and the documentation. In my mental model "error" and "b.c" would be shown.

Problem imho: Reactivity. You can't really depend on the order of execution and if those cases start to pop up, it might get really confusing.

So I might correct myself and would argue for a model of an all-embracing try-catch-block as it is far easier to teach and to reason about. In such a model "error" would be true and "b.c" would be hidden.

[schicks]

While it might be nice for the hook to allow components to "partially recover", it does seem like it introduces a lot of foot guns both for users and for the implementation. An onError hook that ran and stopped the component would be really valuable to me. If fallback UI was needed, the hook could always dispatch a message to the parent who could handle fallback UI. I'm a little worried that partial recovery is letting perfect be the enemy of the good.

[rster2002]

If components were to be implemented so you cannot recover, I would change the slot behavior so a parent component catches errors from components slotted within it as this would allow you to still create some kind of error boundary.

[kyrylkov]

Hey @rster2002. What's the status of this? Can you use some help?

[rster2002]

Haven't been very active lately because of personal life, but help is always welcome of course.

[kyrylkov]

@rster2002 Is this guaranteed to land once completed? If so, we'll be happy to contribute.

[rster2002]

It has been a long open issue, but haven't gotten any feedback from the core maintainers themselves, so might not get merged in the end.

[kyrylkov]

@rster2002 But you started working on this PR. Is there a way to get this greenlighted in advance in principle? Who would be responsible for this?

[rster2002]

People from the core team would probably be responsible for accepting and greenlighting a feature.

[kyrylkov]

@rster2002 Right, but can we somehow get this pre-approved? We don't have resources to spend in vain, but if we get a greenlight from the core, we would be interested to contribute as much as we can. As you know this is a big PITA. We would have to go back to Vue for our next project, if this is not addressed.

[rster2002]

@kyrylkov I'm not part of the core team, you're asking the wrong person.

[kyrylkov]

@rster2002 I understand. I just assumed that you had some assurances before embarking on the journey with this PR.

[kyrylkov]

@antony Could you chime in?

[Crisfole]

@kyrylkov PRs are a great way to communicate an idea and hash out issues with ideas...I think that's more the goal here.

[kyrylkov]

@Crisfole No argument here. My concern is how fast it's moving forward and how we can accelerate it. It looks like interest from the core team is well below what it should be. Especailly taking into account how serious and disruptive this issue is.

So let me rephrase this question. RFC hasn't been merged, while PR looks mostly finished. How do we proceed from here?

[kyrylkov]

@Conduitry Hopefully you can chime in since I cannot seem to get @Anthoy involved

[nimmolo]

@rster2002 maybe it would be a good idea to create an RFC for the scope of this feature PR, specifically. An RFC may attract more comments from other contribs that way, since it's attached to an active PR... an actively discussed RFC seems like a prior requirement for consideration by core.

@kyrylkov if you're referring to the RFC by antony, it seems it is for a somewhat different / complementary feature, a React-like "error boundary".

Edit: there's already been some discussion about this in the PR for antony's "error boundary" RFC that i overlooked. my bad, thanks bluwy for that link

[bluwy]

To give my 2c with the current implementation, I think we should stick to catching errors from descendants only. Catching errors within itself is tricky, which was discussed before. That way, fallback UIs should be easy to implement too without a special return statement. That's how most framework's work and probably for the same reason.

I also think this is worth a separate RFC than the current one. It's a bit different, and we could use a better explanation of how this current API works. Sort of like preliminary documentation.

[rster2002]

I will look into creating a separate RFC. I'm quite busy so this might not happen soon.