rfcs icon indicating copy to clipboard operation
rfcs copied to clipboard

Error Boundary RFC

Open antony opened this issue 3 years ago • 49 comments

rendered

antony avatar Feb 09 '21 10:02 antony

I love that you're pushing this forward. If this is implementable as a directive, though, could it also be implementable as a lifecycle hook? I don't see a significant technical difference between:

<svelte:error handler={handleError} />

and

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

  onError(handleError);
</script>

But the second feels more svelte-y to me: an error occurring is a natural part of the component lifecycle in my mind. Undesirable but natural.

Crisfole avatar Feb 09 '21 13:02 Crisfole

That's another option, yeah. I don't know the implementation detail impact between the two.

antony avatar Feb 09 '21 14:02 antony

It's a great idea, whatever the syntax used.

I'd like to add that it's not possible to properly implements this "in userland" today because there is no API for HOC component now in svelte, but it's probably another subject here.

j3rem1e avatar Feb 09 '21 18:02 j3rem1e

Just been informed about this existing RFC which is a different approach (and IMO has some drawbacks regarding how a handler would be called since it is strictly presentation layer)

antony avatar Feb 09 '21 19:02 antony

I think both the directive and lifecycle approaches are preferable over the template approach.

The tricky part both with the existing error boundary lib (that I wrote) and I presume all these other options is how to reliably catch errors further down the tree.

The current lib catches errors originating inside the child slot, as well as any errors that occur in children as a result of an update originating in or "passing through" said slot. Updates caused by internal state changes inside child components are not always caught tho.

See https://github.com/CrownFramework/svelte-error-boundary/issues/3 for details.

jonatansberg avatar Feb 10 '21 13:02 jonatansberg

@jonatansberg My intuition on this is that the onError lifecycle hook (or directive, which in my mind is functionally the same) should be able to return the error to continue passing it up the tree, or handle it in code. By default any errors just get passed up the tree. If an error reaches a component without a parent then behave exactly as you do now when you hit an unhandled error.

Crisfole avatar Feb 10 '21 13:02 Crisfole

The current lib catches errors originating inside the child slot, as well as any errors that occur in children as a result of an update originating in or "passing through" said slot. Updates caused by internal state changes inside child components are not always caught tho.

It might be interesting if you have some concrete examples of what isn't caught, so that we can look into considering these for this implementation discussion.

antony avatar Feb 12 '21 18:02 antony

@jonatansberg My intuition on this is that the onError lifecycle hook (or directive, which in my mind is functionally the same) should be able to return the error to continue passing it up the tree, or handle it in code. By default any errors just get passed up the tree. If an error reaches a component without a parent then behave exactly as you do now when you hit an unhandled error.

Yeah, that seems reasonable. Although maybe it would be better if you have to re-throw it since it's easy to introduce accidental implicit returns with arrow functions.

The main problem with the current implementation is that errors do not bubble through the component tree but rather the call stack. These two may or may not be the same.

The current lib catches errors originating inside the child slot, as well as any errors that occur in children as a result of an update originating in or "passing through" said slot. Updates caused by internal state changes inside child components are not always caught tho.

It might be interesting if you have some concrete examples of what isn't caught, so that we can look into considering these for this implementation discussion.

There is an open issue here CrownFramework/svelte-error-boundary#3 with a REPL repro. I don't have much more details than that I'm afraid as I haven't had the time to look in to further.

Intuitively it makes sense tho as the current error boundary monkey patches the internal component lifecycle methods in order to catch errors. Updates further down the tree that are "self contained" are not affected by this.

One way to address that would be to catch all errors by default and then use the context to propagate errors up through the component tree rather than the call stack.

jonatansberg avatar Feb 15 '21 13:02 jonatansberg

How about a <svelte:this on:error/> that also uses event forwarding? Error delegation is a plus. This would be breaking, but I think it would be more forward compatible as in <svelte:this on:futureproof/>, and possibly backward compatible <svelte:this on:mount on:beforeupdate/> allowing forwarding other events too.

Separate RFC? What do you think?

milkbump avatar Feb 20 '21 00:02 milkbump

@antony

It might be interesting if you have some concrete examples of what isn't caught, so that we can look into considering these for this implementation discussion.

Here's a REPL with an adapted example from your RFC. TL;DR: any error involving nested components will freeze the app as usual. Doesn't matter if you put the content inside a slot or if the error will happen in nested component's markup.

Essentially, as a developer, I have multiple use cases for error boundary:

  1. wrap my whole app so I can log errors and provide generic error UI in case something goes horribly wrong;
  2. wrap big chunks of components, like a whole section of self-contained UI, to make errors more restricted to a part of the site, while all the other parts would still work. Simple example would be, say, if I wanted to always have working navigation, even if current section of my app threw an unexpected error;
  3. wrap 3rd party components, that I can't control — because there's no other way to work with their possible errors.

All of these cases cannot be solved with current implementation and rely heavily on catching errors all the way down the tree. Current implementation won't help with Kit/Sapper issue as well, because it relies on slots, so there would be no way of restricting errors to a single page/layout.

I would even go as far as to say error boundary loses its value if it only protects you from errors inside current component, because most of the time you can predict possible error states of it and handle them properly with simple a {#if obj?.data}.

Additional

Here's a quote from React's Error boundary documentation, which I can relate to. The idea is to make sure that this portion of tree never crushes the app in general.

Error boundaries are React components that catch JavaScript errors anywhere in their child component tree, log those errors, and display a fallback UI instead of the component tree that crashed.

dkzlv avatar Mar 16 '21 14:03 dkzlv

As to the initial API proposal. I saw in some issues the idea to wrap markup with new {#try}{:catch} template expressions. I find it limiting: you may want to do more stuff than just show some fallback UI; or your "fallback UI" is just a toast that is outside of this component's scope at all!

But I really like the idea of wrapping some parts of the code inside a new expression. I think, it would be more versatile if <svelte:error> was a wrapper and not like <svelte:options /> that works with the whole component in general.

Consider this snippet as an example.

<script>
  import CalendarWidget from 'calendar-dep';
  
  let showFallback = false;

  const genericHandler = (e) => {
    // Log error, trigger fallback UI, etc.
  }
</script>

{#if showFallback}
  <input type="date" />
{:else}
  <svelte:error onError={() => (showFallback = true)}>
    <CalendarWidget />
  </svelte:error>
{/if}

<svelte:error onError={genericHandler}>
  // Here goes the content of the app.
</svelte:error>

Not sure if it complicates stuff though. It can be achieved by just separating it into another component.

dkzlv avatar Mar 16 '21 15:03 dkzlv

I'm in favor of a component similar to React's <ErrorBoundary>. In particular, I think providing fallback HTML should be easy / borderline mandatory. See React's note on unmounting in the event of errors.

I think we could do one better: combine it with the functionality provided by <Suspense>.

Each place where a developer put <Suspense> is a place where we know it is "acceptable" to hide a portion of the application until it is ready. Playing around with it this week, I realized those exact same places are also places where it is "acceptable" to hide a portion of the application, because it broke. They're also places where things are very likely to break; async usually means network traffic which means its never guaranteed to work. Looking up people's React examples, I've seen a lot of <ErrorBoundary> wrapping <Suspense>.

dkzlv's example might look something like this:

<script>
  import { Suspense } from 'svelte/suspense'
  import CalendarWidget from 'calendar-dep'

  function genericHandler (e) {
    // Log error, trigger fallback UI, etc.
  }
</script>

<Suspense>
  <input slot="error" type="date" />
  <p slot="loading">Loading Calendar...</p>

   <CalendarWidget />
</Suspense>

<Suspense on:error={ genericHandler }>
  // Here goes the content of the app.
</Suspense>

Something like this might also provide a relatively straight forward path to handling top level awaits in components, lazy loading, or other features.

WaltzingPenguin avatar Mar 19 '21 05:03 WaltzingPenguin

@ADantes It is a nice addition, but Suspense is way out of the context of this RFC, I think. It's less about error handling and more about an opinionated way of handling data transfer (component code, prefetch).

Btw, we already have {#await import('./Component.svelte') then Cmp}<Cmp.default />{/await} for simple lazy loading and Sapper/SvelteKit, that handles the bundling and data prefetch. It more or less solves the issue for me.

I really like the idea of a slot and the idea that failed tree that has uncaught errors should be unmounted completely.

dkzlv avatar Mar 19 '21 10:03 dkzlv

To provide another use case - I'm looping through widgets installed on a dashboard and each will render their own component,e.g.

{#each widgets as widget}
  <header>widget.title</header>
  <svelte:component this={getRenderer(widget.component)} data={widget.data} />
{/each}

There is a growing library of hundreds of widget components, and performing error handling in each is wasteful. Implementing an error boundary in the <Board> component itself is also a way to drive consistency in terms of how a widget behaves when an internal exception is thrown.

I prefer the {#try}{:catch}{/try} syntax, as it doesn't require the mental gymnastics of a convention-reliant showFallback flag, but I appreciate that one-time side effects like genericHandler don't go well with these directives.

Is this currently in development / is there somewhere to track development or contribute?

mattfysh avatar Jun 21 '21 06:06 mattfysh

As to the initial API proposal. I saw in some issues the idea to wrap markup with new {#try}{:catch} template expressions. I find it limiting: you may want to do more stuff than just show some fallback UI; or your "fallback UI" is just a toast that is outside of this component's scope at all!

But I really like the idea of wrapping some parts of the code inside a new expression. I think, it would be more versatile if <svelte:error> was a wrapper and not like <svelte:options /> that works with the whole component in general.

Since handling exceptions in component constructors is relatively trivial without compiler support, I don't think it's worth committing to error handling semantics without covering the entire life cycle. {#try}{:catch} is seductive as a syntax sugar but I don't see a way to implement it that makes sense as Javascript and Svelte simultaneously.

IMO this <svelte:error> tag implemented as a compiler directive is probably the most straightforward way to provide comprehensive error handling.

Take, for example:

/*--------------------- index.js --------------------*/
import App from "./App.svelte";

try {
    const app = new App({target: ...});
    // Do stuff
} catch (err) {
    // ... catastrophic failure ...
    // root `<svelte:error>` directives set error handlers at the end of initialization
    // since any exceptions in App constructor should be handled outside of Svelte
}
<!------------------- App.svelte ------------------->
<script>
    import ExternalFormComponent from "component-library";
    import Component from "./Component.svelte";
    import { onFormError, onAppError } from "./helpers/errorHandlers.js";
    import { onSave, onSubmit, onCancel } from "./helpers/forms.js";

    const onExternalError = async (err) => {
        // Opaque component we hope is well tested, nothing to do but report it and refresh
        // Oh no! PM rushed our dev so our error handler has an error!
        Sentry.whatever(err); // <-- `whatever` is not a function
        // Bubbles up to defaultErrorHandler in Component, then to onAppError
        window.location.reload();
    };
</script>

<svelte:error on:error={onAppError}>
    <Component>
        <svelte:error on:error={onExternalError}>
            <ExternalFormComponent />
        </svelte:error>
        <svelte:error on:error={onFormError}>
            <!-- All exceptions in onSubmit/onSave handled by `onFormError` -->
            <button on:click={onSubmit}>Submit</button>
            <button on:click={onSave}>Save</button>
        </svelte:error>
        <!-- Exception handled by `<svelte:error>` boundary around `Component` default slot  -->
        <button on:click={onCancel}>Cancel</button>
    </Component>
</svelte:error>
<!-------------- Component.svelte ------------->
<script>
    import { errorNotifications  } from "./helpers/state.js";

    export let title = "";

    const defaultErrorHandler = (err) => {
         $errorNotifications = [...$errorNotifications, { title, message: err.message }];
    };
</script>

<svelte:error on:error={defaultErrorHandler}>
    <slot />
</svelte:error>

When the Svelte compiler encounters a svelte:error directive, it does several things:

  1. it pushes the error handler onto a stack that behaves semantically similar to how contexts do now
    • All svelte public runtime functions like onMount/onUpdate/etc can check this stack and wrap the user closures
    • This allows dynamic components (<svelte:component>) to check for error handlers (if theres no error handler, take the much more efficient path)
    • Provides a way for libraries to interact with error boundaries if they need to support some exotic API
    • Allows nested exception handlers, similar to async control flow
promise
    .then(() => {...}, () => { /*...handleError...*/ })
    .catch(() => { /*...handle error in error handler...*/ });
  1. flags child elements so the compiler knows to generate try/catch wrapper around any inlined code
  2. Passes the error handler to Svelte components instantiated inside the directive so the children know to take the slower code path that provides error handling
    • Theoretically, they can just check the error handler stack in (1) but I suspect it's easier and more runtime efficient to generalize the internal compiler machinery for (2) to support (3)

Off the top of my head:

Pros:

  • Catches all errors that could originate from the code generated by a .svelte file
  • Can handle errors in external libraries without forking and modifying them to wrap each entrypoint

Cons:

  • It's not quite clear how to handle errors in event handlers for custom and passthrough events: should the error handler stack at the dispatch callsite handle the error, at the passthrough site, or at the concrete function handling the event?
  • Implementation complexity: Any Svelte internal that call into user code must check for an error handler, including DOM event listeners, dispatcher, animations, actions, etc.
  • Semantic complexity: This will catch errors that happen anywhere in a component tree. At a high level, this probably makes sense to most people since there's little baggage behind compiler directives, but this behaves far more like exception handling in promises (without async/await) which is definitely not as straight forward as sync code.

akiselev avatar Jul 03 '21 01:07 akiselev

I've created a draft PR (https://github.com/sveltejs/svelte/pull/6585). Of course nothing is set in stone, but I've opted to implement the onError because of a couple of reasons:

  1. onError would allow you to catch errors that occur in the initialization of the component. You would not be able to recover from this, but would provide logging functionality
  2. This would be more general purpose while the other methods I've seen I feel are more specialized
  3. Doing it this way would allow you to create your own component to as error boundaries*

The third point does need some discussion when handling slots:

<script>
import Wrapper from "./Wrapper.svelte";

let a = {}
</script>

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

What would be preferable in this case if onError were to be implemented? Would it be expected to handle the error in the Wrapper component or in the current component.

Again, even though I already created a draft PR, it's just to explore the implementation and would like your thoughts in this.

rster2002 avatar Jul 28 '21 10:07 rster2002

I think about slots as a part of the providing component... If an error occurred in a slot I'd expect the onError of the parent (provider) component to handle it. I could see the case for making that configurable, but....

Crisfole avatar Jul 29 '21 02:07 Crisfole

So I think that the idea is to both implement onError and a error boundary as they would have different behavior. onError would catch all errors within the component regardless of where in the tree is was thrown. <svelte:error> would have my pick for behavior more in line of the original RFC request for an error boundary as it would provide a JS callback with on:error.

rster2002 avatar Jul 29 '21 06:07 rster2002

@Rich-Harris @Conduitry @dummdidumm @benmccann @TehShrike @orta @benaltair

It's a huge unresolved issue for Svelte community: https://www.reddit.com/r/sveltejs/comments/q068rn/so_how_do_people_avoid_svelte_crashing_the_whole/

How do we resume activity on this RFC?

kyrylkov avatar Oct 04 '21 14:10 kyrylkov

@kyrylkov Please do not tag all the maintainers, it is unnecessary and impolite. This RFC is on our radar and will be dealt with as time allows.

antony avatar Oct 04 '21 14:10 antony

@antony I apologize, but this seems to be the only way to get attention. Last week I tagged 2 core team members and got no replies. I'm not asking for active contribution. I want to understand what needs to be done to accelerate the progress. Because as time allows is not good enough. It's literally been years already. And it's a huge issue.

So please let me know.

kyrylkov avatar Oct 04 '21 14:10 kyrylkov

Don't tag any maintainers, ever. We see all issues, and we allocate our own, volunteered time where possible.

I'm not sure what "is not good enough" refers to exactly, but when addressing a team of volunteers who are building something on their own time and money, for the benefit of the community, I don't think it's appropriate.

Please don't further pollute this RFC, if you want to chat about the contract between the svelte core team and yourself, then please come and find me in discord.

antony avatar Oct 04 '21 14:10 antony

To give a bit more detail on this, it is something that we've discussed at the monthly maintainers meeting. Rich had some ideas for rewriting a lot of the Svelte internals that would allows us to address this in a nice way, make hydration improvements, and make a number of other improvements. We'd like to consider those ideas in more depth as a possible solution. This is a very core part of the language, so it's important to us that we get the best solution even if that takes longer. I understand that it's frustrating not to have better error handling in the meantime of course, but think that's best for the long-term success of Svelte to do it right rather than quickly. I'm afraid we're unlikely to have many other updates on the topic for awhile. We're quite busy for the next couple months with other priorities such as getting to SvelteKit 1.0 and preparing for the upcoming SvelteSummit. I think we're likely to refocus on Svelte core for awhile after getting SvelteKit 1.0 out the door

While it's not the error boundary feature, I do recommend folks interested in improve error handling upgrade to the latest version of Svelte, which does have improved error handling for #await blocks: https://github.com/sveltejs/svelte/blob/master/CHANGELOG.md#3431

benmccann avatar Oct 04 '21 17:10 benmccann

What Rich has in mind would result in Svelte 4 or still Svelte 3? Is it worth putting effort into https://github.com/sveltejs/svelte/pull/6585 or it has no chances of landing so we should just really give it more time and wait until after SvelteKit 1.0?

kyrylkov avatar Oct 04 '21 17:10 kyrylkov

Should I continue to work on #6585 or drop it as this isn't something that would be considered to be merged?

rster2002 avatar Oct 06 '21 20:10 rster2002

I can't really say, but I would say that the normal process is that the RFC document is agreed upon and merged first, which indicates approval of the feature, and then most of the work on the feature is completed after that. Sometimes a PR can be helpful for discussion purposes to show what the code and feature would look like if you're okay with the fact that it might not be accepted, but I certainly wouldn't go as far as polishing all the rough edges without the RFC here getting merged first.

benmccann avatar Oct 06 '21 20:10 benmccann

I always find the awkward edge cases easier to suss out with code in front of me...i mean, just the conversation around "what would you expect here" had been valuable in my eyes. Maybe other folks are more talented than I am in that regard (I would totally believe that).

Crisfole avatar Oct 08 '21 00:10 Crisfole

Is there a workaround? I just discovered an open source package from npm that is a Svelte component, is throwing exceptions in prod, which causes the frontend app to freeze.

mattfysh avatar Jun 01 '22 07:06 mattfysh

Any update on this yet? Still a massive pain

wvanrensburg avatar May 04 '23 20:05 wvanrensburg

I think I'm missing something here... how is this still not implemented?

What are people doing if you install a component from npm and it throws while it renders? Currently the entire app freezes, and there is no opportunity to recover and display a fallback. Your user simply can no longer use the page until they refresh

This is akin to releasing a new programming language without try/catch - not good enough

mattfysh avatar May 05 '23 01:05 mattfysh