sveltejs / svelte

Cybernetically enhanced web apps
https://svelte.dev
MIT License
78.19k stars 4.09k forks source link

Feature suggestion: error handling API #1096

Open jbmoelker opened 6 years ago

jbmoelker commented 6 years ago

This is a feature suggestion for adding an error handling API to Svelte components based on this remark.

There is currently no built-in way to handle errors in Svelte components. React 16 introduced the concept of Error Boundaries to standardise this:

// Error handling life cycle in React:
componentDidCatch(error, info) {
    // Display fallback UI
    this.setState({ hasError: true });
    // You can also log the error to an error reporting service
    logErrorToMyService(error, info);
  }

I imagine a similar concept would be a good addition to Svelte components. Maybe

declaratively, as event on element:

<MyComponent on:error='doSomethingWith(e)' />

imperatively, as component life cycle event:

oncreate() { /* ... */ },
onerror(e) { /* ... */ },

I would check if the error is handled inside the component - with the onerror handler - first, and only if it isn't handled there, have it bubble up and trigger on:error. If a developer wants to handle the error inside the component and also wants to have it bubble up, they can use this.fire('error', e) within onerror.

trenta3 commented 5 years ago

I also would love error handling so much. The problem with the current situation is that an error in whichever component completely blocks the UI. Related to error handling, it could be useful if a setting was given to "restart" components in case of error up to a maximum numbers of errors. Another useful addition would be to "block" only the component code that raised exception.

Crisfole commented 5 years ago

To fit w/ svelte v3 I think this should probably be just like the rest of the lifecycle hooks:

import { onError } from 'svelte';

onError(error => {
  Rollbar.report(error);
  return true; // Truthy to indicate handled, falsey to continue bubbling the error to the top level.
});

Internally this would do something like this:

export function onError(fn) {
    get_current_component().$$.on_error.push(fn);
}

And each component that imports onError would get its update code wrapped like so:

update = wrap(CURRENT UPDATE CODE);

Where wrap is shared code that looks like this:

function errorWrap(updater) {
  return function (...args) {
    try {
      updater(...args)
    } catch (err) {
      if (!get_current_component().$$.on_error.some(handler =>handler(err))) {
        throw;
      }
    }
  }
}

This is mostly a sketch here...

antony commented 5 years ago

I like this for two reasons:

Gets my vote.

carcinocron commented 5 years ago

Here's the docs on Vue's implementation https://vuejs.org/v2/api/#errorCaptured

trenta3 commented 5 years ago

Since it seems very important to have and easy to do (just wrap each component in a try catch as per @Crisfole sketch), can we expect it will be implemented soon or at least added to a milestone? Thanks for the great work.

Crisfole commented 5 years ago

@trenta3, let's not oversimplify here. Every feature requires a design, implementation, tests, and documentation. Even the implementation is bigger than just adding a try catch, you have to handle bubbling, adding event handlers, making any changes to parsers and code generators, writing the 'onError' function and all supporting code, etc.

I'm very much I'm favor of this, but it's certainly not a done deal. Even the design is under specified at this time.

trenta3 commented 5 years ago

Sorry if I sounded rude: it was not my intention to oversimplify things. I just wanted a sort of reassurance on it getting implemented, since it is for me mandatory before using svelte in production environments and the issue wasn't getting proper attention since January 2018. Sorry again.

Crisfole commented 5 years ago

No, I didn't think it was rude. Just wanted to be clear that even though I love my own suggestion! ( :D ) it's not actually super simple to implement

lastqubit commented 4 years ago

Writing a component like this would be nice:

<svelte:head>
    <title>Title</title>
</svelte:head>

<svelte:error>
    <h1>Opps error</h1>
</svelte:error>

<div>My component</div>

only svelte:error renders if there are any errors

antony commented 4 years ago

@lastqubit I think the idea is to add error handling in component script code, rather than have a new svelte tag. The scope of a <svelte:error> tag is too narrow, as well as being cumbersome to use (how would error messages be passed, etc).

An onError lifecycle hook would allow for reporting, handling, and display, among other things, which a tag would not.

lastqubit commented 4 years ago

@antony Without a svelte:error component everyone has to write the boilerplate code to display errors and that doesn't fit with sveltes strength of being user friendly.

If you somehow can get the error object in the error component it's very elegant. Everyone has to handle errors, so to have a specialized component that's very easy to use is not a narrow scope in my opinion. svelte:error becomes a more user friendly and elegant version of ErrorBoundary in react.

You can log like this:

const app = new App({
    target: document.body,
    onError: (e) = > console.error(e)
})

Without my suggestion you have to wrap every component in an if statement(very ugly)

<script>
    import { onError } from 'svelte'
    let error
    onError((e) => {
        error = e
    });
</script>

{#if error }
    <div>Opps error...</div>
{:else}
    <div>My component here</div>
{/if}
antony commented 4 years ago

@lastqubit I feel that is very lazy error handling. Errors should be dealt with on a case by case basis, and only thrown to a global handler if there really is no way for the user to recover.

Boilerplate is only boilerplate if it's the same everywhere, which it shouldn't be. Additionally, if you're writing a notification display in every single component, wrapped in a <svelte:error> tag, that's the very definition of boilerplate.

Here's how I'd deal with errors in my components:

<script>
import { onError } from 'svelte'

onError(e => {
  // can I handle it automatically? do so
  stuff()
  // should I be informed about this?
  reportingApi.send(e)
  // should the user know about it?
  notifications.send('Some problem occured', e) 
})
</script>

and if I think this is too boilerplatey, I can export a handler from some .js file and pass the error to that:

<script>
import { onError } from 'svelte'
import { genericHandler } from '../my-error-handler.js'

onError(genericHandler(e => {
  // code which is called first to try to handle this locally
  return true // we've handled it here, don't do anything else.
})
</script>
lastqubit commented 4 years ago

@antony My example is more like ErrorBoundary in react, where your entire component should become an error page on exceptions. Your code handles errors by notifications and doesn't really show the code for actually displaying errors as html.

I see alot of value in a svelte:error component whose only purpose is to remove that extra ugly wrapping {#if} statement for displaying error pages.

You can have both an onError and svelte:error and choose what fits your needs.

antony commented 4 years ago

@lastqubit I didn't want to include my entire notifications library in my example, but I use this: https://www.npmjs.com/package/@beyonk/svelte-notifications

I'm not averse to both a component and a handler, however I feel that having both could cause some confusion as to who handles the error first? The code, or the view component?

lastqubit commented 4 years ago

@antony The code handles first, and you get a chance to transform the error object. I don't know what syntax to use to get the error object in the component but it would be nice if you didn't have to declare it yourself in the script tag

<script>
    import { onError } from 'svelte'

    onError((e) => {
        console.error(e)
        return {
            statusCode: 500
        }
    });

</script>

<svelte:error as error>
    <div>Opps error {error.statusCode}</div>
</svelte:error>
antony commented 4 years ago

I don't think there's a need for a <svelte:error> tag. It could be implemented trivially (and simply - a better solution might use contexts) thus:

{#if error}
<h1>An Error! {error.message}</h1>
<pre>
  {error.stackTrace}
</pre>
{:else}
<slot></slot>
{/if}

<script>
  import errorStore from './error-store.js'

  const unsubscribe = errorStore.subscribe(value => {
    if (!value) { return }
    error = value
    errorStore.set()
  })

  onDestroy(unsubscribe)

  let error = null
</script>
lastqubit commented 4 years ago

@antony That's like the example i wrote, and i think it's very ugly. It's annoying when frameworks are very elegant in demos and presentations but then that elegance disappear when you have to write real world code.

Everything is trivial in react to, but when you constantly have to write way more ugly code than needed it becomes a mess pretty quickly.

If i wanna write a simple image component that renders differently on exceptions i should write that code everytime instead of just adding svelte:error and render my error code in that?

antony commented 4 years ago

@lastqubit The example above would not be repeated at all, it would be a component:

as a "boundary" style component:

<ErrorBoundary>
  <h1>Your other code</h1>
</ErrorBoundary>

or as a simple error notification:

<ErrorDisplay />

Roughly equivalent to your suggestion of <svelte:error>.

An annoying thing about frameworks is when they get too opinonated, which is, in my view, a problem React has.

lastqubit commented 4 years ago

@antony I don't understand why everyone should write their own error boundary component when svelte:error would be perfect. I don't understand why svelte:error would be opinionated and svelte:head for example is not.

It's very easy to change title in react but i love svelte:head. It's very elegant and right away you get a perfect understanding of your component, svelte:error would continue that tradition instead of having to write that annoying boiler plate code that everyone who starts using svelte have to write.

<svelte:head>
    <title>Title</title>
</svelte:head>

<svelte:error>
    <h1>Opps error</h1>
</svelte:error>

<div>My component</div>

is very elegant and right away you understand the component. The code variant is a mess in comparison.

Crisfole commented 4 years ago

@lastqubit So, I think the issue is that it's not totally perfect. It doesn't define what should happen in parent components, it's not as flexible as onError and it doesn't allow you (for instance) to nest a svelte:head inside, or decide what to do with the rest of the rendering. What do you do with <div>My component</div> in your example? What about changing the <title>? I assume you can inspect the error...does <svelte:error> allow specifying which error types to expect?

With an onError function those questions are left in the capable hands of the coder. onError just becomes a lifecycle function that interacts with the rest of svelte seamlessly.

Here I feel like I must re-iterate that the implementation of this is non-trivial...

Crisfole commented 4 years ago

One last comment here:

It's annoying when frameworks are very elegant in demos and presentations but then that elegance disappear when you have to write real world code.

Unfortunately the real world is tricky. Especially handling errors which can be for any one of an unlimited number of reasons. I don't think you'll find that the elegance has actually gone, though, just because a convenient piece of syntactic sugar is not present. Having a consistent and predictable pattern is key to the elegance.

Maybe once the core onError lifecycle is implemented (if maintainers decide to go that way) everyone will discover you're right and an implementation will be built in. I think that's probably what's going to happen. But until real life has proved it, it's usually best to go for the smallest most broadly applicable solution. I can definitely imagine <svelte:error> eventually being a thing, but it's a pretty dramatic change compared to an added importable function.

bdoms commented 4 years ago

Not having some sort of built in handler for errors is really painful, as I just discovered today. I have a window.onerror handler that was working fine, but I noticed today that an error occurred and it wasn't called. It's scary to think about an error happening and the fact that you might not know about it.

After some digging, I found out that the culprit was this line: https://github.com/sveltejs/svelte/blob/1273f978084aaf4d7c697b2fb456314839c3c90d/src/runtime/internal/scheduler.ts#L17 resolved_promise.then(flush);

When an error is thrown inside a Promise it looks like Firefox still calls window.onerror, but Chrome swallows the error silently. I had to add a window.onunhandledrejection handler for this special case - I don't use any Promises in my own code, so I never thought I'd have to include something like that. For anyone finding this page with similar issues: even after adding that handler, Chrome would display the error in the console, but never trigger it while I was accessing the code over a hot reload web socket. I had to build a whole file and run it as if I were in production before Chrome actually triggered that other handler.

That last piece with the websockets is probably outside svelte's purview, but if svelte had provided some sort of onError handler I could've attached a function to, then it would simplify things for me, and prevent other developers from having to track down similar issues.

Ideally that same line above - and everywhere else svelte uses Promises like this - becomes something like resolved_promise.then(flush).catch(onErrorHandler) - that way there's one handler for everything from my perspective as a svelte user, and I don't have to worry about whether svelte has some internal promise that doesn't have a catch on it or not.

RedHatter commented 4 years ago

This would be a very useful feature.

happybeing commented 4 years ago

Learning svelte I landed here looking for guidance on handling errors and think the onError() approach looks promising.

In the mean time something in the REPL or tutorial about this would help newcomers like me.

eddpeterson commented 4 years ago

If at least one component has smallest unhandled error, the whole app will crash and users will not know what to do and developers will not know such an error occurred. @bdoms solution seems most pragmatic at the moment (thanks), but I hope more graceful error handling comes soon enough.

Here is the code snippet of how I am using it at the moment: https://svelte.dev/repl/c6dddc73cbdd4f81883add43f5e3aa25?version=3.18.2

carcinocron commented 4 years ago

Important to note that the snippet https://svelte.dev/repl/c6dddc73cbdd4f81883add43f5e3aa25?version=3.18.2 will not work for all types of errors, and there's no way to get something like Sentry to be able to record all errors and/or show a proper error message for all errors, so your app just freezes and if its' your users experiencing the error you won't even know about it. You can't write cypress tests for business logic edge cases and browser incompatibilities you don't know about.

HazAT commented 4 years ago

Just make sure to call the original onunhandledrejection after you are done, otherwise we (Sentry) are not able to pick up the error.

onMount(() => {
    const originalUnhandledRejection = window.onunhandledrejection;
    window.onunhandledrejection = (e) => {
          console.log('we got exception, but the app has crashed', e);
          // or do Sentry.captureException(e);
          originalUnhandledRejection(e);
    }
})

Another option is to do nothing at all and just call Sentry.init, our SDK is able to pick up most errors.

antony commented 4 years ago

I think this would make a great RFC, if there is appetite.

cdhanna commented 4 years ago

I just ran into this issue today, and realized that there isn't any way to save my application if one component goes haywire. I'll be watching this issue for some sort of feature implementation.

frederikhors commented 4 years ago

Please guys, this is very huge!

burningTyger commented 4 years ago

I have mentioned something along these lines in the future channel once:

{#try}
 {some_failed_code}
{:catch}
  Caught..
{/try}

This obviously doesn't stop the component from failing if it has a problem elsewhere but you can still catch some bugs if you receive e.g. faulty input.

ConProgramming commented 3 years ago

Feel like this should be talked about more... how are people doing logging and making sure errors don't just freeze the whole page?

jonatansberg commented 3 years ago

There is a related issue here: https://github.com/sveltejs/svelte/issues/3733

I've created a basic error boundary component (based on a previous proof-of-concept by @halfnelson) available here: https://svelte.dev/repl/9d44bbcf30444cd08cca6b85f07f2e2a?version=3.29.4

ghost commented 3 years ago

For those of you that want an easy way to implement @jonatansberg's library - the guys over at Routify made a way to wrap it around every page using decorators

antony commented 3 years ago

FWIW it looks like this would also help Sentry provide a specific Svelte integration which would be very handy.

https://twitter.com/DanielGri/status/1333742329363050499

floer32 commented 3 years ago

There is a related issue here:

3733

I've created a basic error boundary component (based on a previous proof-of-concept by @halfnelson) available here: https://svelte.dev/repl/9d44bbcf30444cd08cca6b85f07f2e2a?version=3.29.4

Curious, has anybody tried calling Sentry or Log Rocket using that onError prop/ functionality, that you implemented? (I intend to try it, just split attention at the moment)

jonatansberg commented 3 years ago

There is a related issue here:

3733

I've created a basic error boundary component (based on a previous proof-of-concept by @halfnelson) available here: https://svelte.dev/repl/9d44bbcf30444cd08cca6b85f07f2e2a?version=3.29.4

Curious, has anybody tried calling Sentry or Log Rocket using that onError prop/ functionality, that you implemented? (I intend to try it, just split attention at the moment)

Yes, that’s exactly how we use it :)

antony commented 3 years ago

RFC here: https://discord.com/channels/457912077277855764/750401468569354431/808649180854747156

nickolasgregory commented 3 years ago

For those without discord, that item links to;
https://github.com/sveltejs/rfcs/blob/4940d243733b284e93b154c2d3b989b2b396cda9/text/0000-error-boundary.md
Which I guess is;
https://github.com/sveltejs/rfcs/blob/rfc/error-boundary/text/0000-error-boundary.md

antony commented 3 years ago

Whoops, my bad - copied the link from discord. thanks @nickolasgregory

lastqubit commented 3 years ago

@antony I'm glad you listened and implemented my idea in svelte kit

al6x commented 3 years ago

An issue with alternative solution as try/catch looks like a better design.

rster2002 commented 3 years ago

I've taken the initiative to start and implement the onError idea as it's the simplest and would allow for more specialization down the line, but I have a question about what the expected behavior would be in your opinion in the following example:

<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}

In this case, would you expect the behavior to be in this case? Should the 'Caught' message show? {#if b.c} would also throw because it's value is set after the error so what would you expect to happen there?

A case can also be make for not mounting the component at all because it would make it clearer for the developer what is being shown after an error and what isn't but this also defeats the purpose a bit of having onError.

Just let me know any of your thoughts on this.

98mux commented 3 years ago

@rster2002 Thoughts about this approach?

<script>
    import { onError } from "svelte";
    import ErrorComponent from "./ErrorComponent.svelte";

    let a;
    let b = {};
    onError(() => {
        return {component: ErrorComponent, props: {}};
    });

    a.b; // Error is thrown here

    b.c = true;
</script>

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

The onError return value will render when there is an error. You could use

<svelte:component this={component} {...props}/>

To render the error component behind the scenes.

Fewer amount of lines, easier to refactor and reuse

rster2002 commented 3 years ago

Interesting approach. So this would basically replace the current component with the ErrorComponent in this case. What would happen if you didn't return something in onError? Would the current component not mount or should something else happen?

98mux commented 3 years ago

@rster2002 Yeah, i guess you could just show nothing then. Or maybe, there could be some default fallback ErrorComponent with some basic information about the error.

98mux commented 3 years ago

@rster2002 I'm not sure if it is possible, but if you solved the error you could return this, to attempt again? Probably would give kinda weird syntax

    onError(() => {
        if(solved){
            return {component:"this", props: {}}
        }
        else{
            return {component: ErrorComponent, props: {}};
        }
    });
rster2002 commented 3 years ago

It would probably be possible, but this would make it very easy to create infinite loops where the component fails initializing no matter what.

98mux commented 3 years ago

@rster2002 With some compiler magic, it could replace the onError implementation for all components returned within onError. It would replace it with an onError that returnes the default fallback ErrorComponent, with additional text such as "The returned component from onError also had an error". Such that onErrors only gets 1 level deep.

Thought that would definitely complicate things haha :P

basaran commented 3 years ago

while return {component: ErrorComponent, props: {}}; looks nice, it also feels reacty and vuey.

I like @eddpeterson

https://svelte.dev/repl/c6dddc73cbdd4f81883add43f5e3aa25?version=3.18.2

approach to be better. It's vanilla JS, doesn't bind you to specifc syntax and that's the main reason why I like svelte that it doesn't try to sandbox you into framework constraints.