Failing test for onError handler.

[Crisfole]

Starts speccing out solution for #1096: adding an onError lifecycle method. It basically adds a single failing test and some supporting code around it.

Rather than always adding a try/catch this adds a try/catch only when on_error handlers are present. (Addressing a concern from Discord about not causing performance penalties when not being used).

The test fails because apparently updates and initial renders go through different code paths and I can't find the init path. A document explaining the general code path the compiler produces would be really helpful.

[Rich-Harris]

Belatedly: I really like this idea, I think it would be an important addition. And I think the API is a good one, and that the implementation is as good as it could be given Svelte's current shape.

One reservation I have is that we still wouldn't have a good way to deal with corrupted state. If an error occurred during rendering, we wouldn't be able to roll back to the last good state once we'd handled the error. So while this would be good for telemetry and diagnostics, I'm not sure if it gives us what we'd ideally want in terms of making apps more resilient to errors during update.

For that to happen, I think we'd need to do the same work that would be required to implement any form of suspense (#1736) — instead of component updates resulting in changes being applied to the DOM immediately, we'd need to queue them up (and then either run or discard the queue depending on what happened). That's obviously a major change that we'd need to think carefully about (we might also need a way to roll back state).

The test fails because apparently updates and initial renders go through different code paths and I can't find the init path

It happens inside the init function:

svelte/src/runtime/internal/Component.ts

Line 144 in aa0341c

$$.fragment = create_fragment ? create_fragment($$.ctx) : false;

That said, I'd like to find a way to unify init and update at some point, to reduce code duplication.

So I guess the question is whether we push forward with onError in the way we're currently able, or plan for a (uncertain) future in which it would have different (but probably better) semantics?

[Crisfole]

Worked at Rollbar for quite a while; I'm biased toward support for telemetry and diagnostics. That being said, I'm not totally sure that you can generically do any better than forcing someone to handle the error.

Returning to the previous state is probably pretty bad UX for most situations, especially if there's no noisy console error logged...

I'm convincable. You've proven several times that solutions I didn't think would work can...

[trenta3]

I think that having an onError handler for at least reporting errors to the diagnostics would be very useful and almost mandatory in a production environment.

This should also allow (but correct me if I'm wrong) to restart the component which threw the error to the initial state, which is a simple and effective rollback strategy. Indeed, in most of the UI I can think of, this would be a great way to treat the error: send it to diagnostics, notify the user that an error occurred and that it will be resolved soon and then restart part of the UI.
I also think that this use case (restart component on error up to n times) should also be included in the svelte tutorial.

[Rich-Harris]

Ok, having something that's mainly intended for telemetry does make sense I guess.

I thought of something else though: IIUC, as currently implemented, error handlers would only be invoked if the error occurs in the component with the handler. So you couldn't have an app-level onError handler and use it to catch problems further down the tree — you'd have to have a handler in every component, effectively.

I think for this to have the desired effect it would have to behave a bit more like the context API — i.e. you call the function, and it sets the error handler for an entire subtree, and all updates are try-catched (try-caught?). (My understanding is that try-catch is no longer a performance cliff, but we'd still probably want to do some exploration about the impact of such a change.)

[Crisfole]

@Rich-Harris As soon as you called out the error handler not working for sub trees I immediately jumped to the context API. Sounds like a strategy like that is definitely the way to go...

I think any change to svelte has to be filtered through its primary goals...and performance is one of the top ones. If this has a performance impact then it needs to head back to the drawing board.

I'm not sure I actually believe that try/catch hurts performance in the happy case (no error is thrown). Some random (unchecked, possibly invalid, not sure yet) performance checks actually suggest that try catch with no error thrown is actually equally performant:

function handle(fn) {
  try {
    return fn();
  } catch (e) {
    return e;
  }
}

function sum(n) {
  return [...Array(n).keys()]
    .map(n => n+1)
    .reduce((acc, n) => acc + n, 0);
}

function errSum(n) {
  throw new Error(`Result: ${sum(n)}`);
}

function timer(title, num, fn) {
	console.time(title);
  const results = [];
  for(let i = 0; i < num; ++i) {
    results.push(fn());
  }
  console.log(title, results[results.length - 1]);
  console.timeEnd(title);
}

// Let the optimizer do its thing...Ignore these results.
timer("Priming - Ignore Results", 500000, () => handle(() => errSum(10)));
console.clear();

// Test the timing:
timer("Try Catch - No Error", 500000, () => handle(() => sum(10)));
timer("No Try Catch", 500000, () => sum(10));
timer("Try Catch - Error", 500000, () => handle(() => errSum(10)));

The unhappy case (where an error is thrown) definitely has poorer performance: unwinding the stack is expensive. You can try reordering these, the order matters (the second no-error case with or without a try/catch always beats the first by a few centiseconds), but the caught error case is an order of magnitude slower. Each loop of 500000 no-error case takes about 270ms on my machine. The error case takes 7000ms.

I haven't thrown in the async and Promise case just yet, but my intuition says it probably doesn't matter in native async land, and might be even less of an issue in Promise land (especially if you're not using native promises, and you're using something like Bluebird instead).

I reserve the right to revoke all my statements in the previous paragraph in light of cold hard data.

[Crisfole]

@Rich-Harris given the performance data, is there any reason to not always try/catch the init and update?

The happy path will be unchanged performance wise, and the error case is necessarily slower, but I doubt if it'd ever be noticeably so.

[benbenbenbenbenben]

Does this need some extra hands on it? I've run into a the scenario where I need this feature...

[jonatansberg]

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

It should work both for local errors and the subtree.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[benmccann]

Thanks for sharing this. I'm going to go ahead and close it since it can't be merged, but it should show up on the referenced ticket so that it can be leveraged if someone has an implementation for the issue

[Crisfole]

Of course. I didn't write it to get it merged, I wrote it to have something concrete to talk about.

This is directly connected with the Error Boundary RFC