Error handling

[zbeyens]

Description
This pull request primarily focuses on enhancing error handling by introducing undefined checks, refining return types for functions, and implementing a global ErrorLogger interface. It also introduces an editor.onError function to manage errors in functions that depend on the editor.

Issue

Fixes #3243
Fixes #3558
Fixes #3568
Fixes #3586
Fixes #3641
Fixes #3705
Fixes #3708
Fixes #3723
Fixes #3777
Fixes #3811
Fixes #3870
Fixes #3878
Fixes #3900
Fixes #3918
Fixes #3948
Fixes #4001
Fixes #4081
Fixes #4088
Fixes #4094
Fixes #4136
Fixes #4323
Fixes #4485
Fixes #4561
Fixes #4564
Fixes #4581
Fixes #4643
Fixes #4658
Fixes #4738
Fixes #4771
Fixes #4784
Fixes #4847
Fixes #4851
Fixes #5014
Fixes #5025
Fixes #5066
Fixes #5229
Fixes #5273
Fixes #5355
Fixes #5375

For a good part of these issues: this PR is only fixing the "crashing" part but not the underlying issue – the error will be stored in editor.errors. So it's not a complete fix, but I think it's important to close these issues until the description is updated (i.e. the editor does not crash anymore).

Major Changes

• Added editor.onError function to handle errors in functions that depend on editor.
• Introduced a global ErrorLogger interface to handle errors in functions not depending on editor.
• No errors are thrown by default, addressing community feedback from issue Slate throws exceptions too liberally in relation to selection failures #3641.
• Updated return types of several functions to include | undefined and replaced throw new Error() statements with either editor.onError() or ErrorLogger.onError().
• Implemented conditional checks for variables before accessing them to prevent crashes and improve code stability.

Example

import { ErrorLogger } from 'slate'

ErrorLogger.addErrorHandler(error => {
  throw new Error(error.message)
})

You can also filter errors by type:

ErrorLogger.addErrorHandler((error) => {
  // handle error
  if (error.type === 'Node.get') {
    throw new Error(error.message)
  }

  // access errors array
  console.log(ErrorLogger.getErrors());
  
  // reset errors array
  console.log(ErrorLogger.resetErrors());
});

Context
The motivation behind these changes is to improve code stability, maintainability, and robustness by providing more error control to users. By introducing the global ErrorLogger interface and the editor.onError function, this pull request aims to make error handling more flexible and manageable. Some behaviors that previously threw an error may now continue to work without crashing the editor, which could potentially cause regression.

Error Handling

Error handling in Slate is designed to provide more control to users while maintaining code stability, maintainability, and robustness. Slate introduces a global ErrorLogger interface and an editor.onError function to handle errors in a more flexible and manageable way. Utilize these error handling mechanisms to tailor your error management based on your specific use case and requirements, ensuring a smoother and more stable experience for your users. For example, you may want to throw errors in development, but in production, you would prefer to have non-blocking errors for your users.

ErrorLogger

The global ErrorLogger.onError function is used to handle errors in functions not depending on the editor, like Node.get. To set custom error handling behavior, call ErrorLogger.addErrorHandler().

To throw errors on invalid operations:

import { ErrorLogger } from 'slate'

ErrorLogger.addErrorHandler(error => {
  throw new Error(error.message)
})

You can also filter errors by type:

ErrorLogger.addErrorHandler((error) => {
  // handle error
  if (error.type === 'Node.get') {
    throw new Error(error.message)
  }

  // access errors array
  console.log(ErrorLogger.getErrors());
  
  // reset errors array
  console.log(ErrorLogger.resetErrors());
});

editor.onError

The editor.onError function is used to handle errors in functions that depend on the editor.

To set custom error handling behavior for the editor, override the editor.onError function.

To throw errors on invalid operations:

editor.onError = error => {
  throw new Error(error.message)
}

You can also filter errors by type:

// throw only for `move_node` operation
editor.onError = error => {
  if (error.type === 'move_node') {
    throw new Error(error.message)
  }
}

editor.errors

By default, Slate pushes errors to the editor.errors, ErrorLogger.getErrors() arrays. You can actively manage this array to keep it empty by implementing your own error handling strategies. This allows you to effectively monitor and address errors that occur during the editor's operation, contributing to a stable user experience.

Migration

If you want to keep the previous behavior, here is the quickest migration using non-null assertion (estimating to a couple of minutes):

Throw an error like before:

import { ErrorLogger } from 'slate'

ErrorLogger.addErrorHandler(error => {
  throw new Error(error.message)
})

editor.onError = error => {
  throw new Error(error.message)
}

Here is the list of APIs that now return | undefined. Find usages for each of these and insert a non-null assertion. For example: Path.next(...) to Path.next(...)!

Path.next
Path.parent
Path.previous
Path.relative
Editor.edges
Editor.end
Editor.first
Editor.last
Editor.leaf
Editor.node
Editor.parent
Editor.path
Editor.point
Editor.range
Editor.start
Node.ancestor
Node.child
Node.children
Node.common
Node.descendant
Node.first
Node.get
Node.last
Node.leaf
Node.parent
ReactEditor.findPath
ReactEditor.toDOMNode
ReactEditor.toDOMPoint
ReactEditor.toDOMRange
ReactEditor.toSlateNode
ReactEditor.findEventRange

The alternative is to wrap each of these into a function that does not return | undefined (alias type) but this would take more time to refactor.

Checks

• The new code matches the existing patterns and styles.
• The tests pass with yarn test.
• The linter passes with yarn lint. (Fix errors with yarn fix.)
• The relevant examples still work. (Run examples with yarn start.)
• You've added a changeset if changing functionality. (Add one with yarn changeset add.)

[changeset-bot]

🦋 Changeset detected

Latest commit: 3be4e90

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
slate	Minor
slate-react	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[SmilinBrian]

Wow, this should be a huge improvement! 🎉 Thank you! 🙏🏼

It's a massive amount of work which I unfortunately do not have time to review in detail this week. What I've looked at so far looks good, although there are a few places/cases that used to crash which now "silently succeed" that I'll have to think carefully whether are worthy of passing to the error handler/logger or not… in some cases it is really hard to decide whether something is "reportable" or not

[Edit for clarity: What I see on a quick overview is that places that used to explicitly throw now result in a callback to error handers we define, and if our handler does not stop operation (throw) then the code just returns false / null / undefined and continues. This is obviously a huge improvement and gives us good choices.

But there is a huge volume of changes where, if the old code would have resulted in throwing "cannot access "prop_xyz" on undefined", the new code also just returns false / null / undefined and continues… in some of those cases I think maybe the error handler should be invoked because maybe it really is a problem, but we're never going to even know that 'something bad' is happening.]

[zbeyens]

Thanks for the review @SmilinBrian ! This is the "possible regression" part I was thinking of. Unfortunately, we don't have unit tests that would help to identify these critical errors. The goal is to have an editor that never freezes on user inputs – e.g. even if the selection is buggy. One place that is obvious to throw is the slate-react hooks. One way of handling a (less) critical error is to early return. On the other hand, we could improve the user experience by avoiding early return for "soft errors" (array queries https://github.com/udecode/slate/blob/63350f8b3828108c70e21db1bedf28b8456193ab/packages/slate/src/interfaces/node.ts#L386).

I think we can just wait for issues to emerge to identify the critical errors.

[BrentFarese]

This is a good amount of code changes so thank you @zbeyens for putting in the effort! I have 1 larger question for the community, which is this the preferred route for catching errors that are thrown by Slate? There a couple downsides of this PR that I'm not too keen on:

1. The type changes that include | undefined are going to result in a lot of breaking changes for folks migrating. The developer now needs to decide whether to throw errors in their own code, or just do nothing and "silently fail" if a given Slate func returns undefined.
2. Because funcs now return | undefined there can be silent failures that should be fixed in the codebase. Is it better to loudly fail so the codebase gets better vs. silently fail? Developers can still handle errors in their code properly w/o crashing their app...
3. Have any other lower-lift/backwards compatible ways of doing this been considered?

On (3), an idea I had (and something we do at Aline) is wrap all Slate queries/transforms in error handling logic, and then only use the wrapped queries/transforms in the app code base. Here is some pseudocode example:

import { Transforms } from 'slate';

type SlateTransformType = typeof Transforms;
type WrappedSlateTransforms = {
  [K in keyof SlateTransformType]: (
    ...args: Parameters<SlateTransformType[K]>
  ) => ReturnType<SlateTransformType[K]> | undefined;
};

export const SlateTransformsWithErrorHandling = Object.keys(Transforms).reduce<WrappedSlateTransforms>(
  (acc, transformName) => {
    const transform = Transforms[transformName];

    return {
      ...acc,
      [transformName]: (...args: any[]) => {
        try {
          return transform(...args);
        } catch (e) {
          console.error(e);
        }
      },
    };
  },
  Transforms
);

Then any dev wanting errors to be caught could use SlateTransformsWithErrorHandling instead of Transforms directly. This might be a decent approach, but haven't thought through all the ramifications.

Basically, changing the type of everything to | undefined I think is going to require a lot of code changes when users migrate and cause some silent errors that are not good, no? It certainly would prevent Aline from migrating until we can dedicate time to make the changes needed in our codebase to handle | undefined.

Thanks for checking out the feedback!

[zbeyens]

Thank you for your feedback and for expressing your concerns. I understand that introducing | undefined in return types may lead to a more challenging migration process for larger projects. However, I believe that including these in Slate core is essential for several reasons:

1. Improved error handling: By returning undefined instead of throwing errors, we provide developers with greater control over how their applications handle errors. They can decide whether to throw errors, handle them gracefully, or log them for further analysis. This flexibility enables more robust and customized error handling strategies in different environments (e.g., development vs. production).
2. Enhanced code stability: Previously, unhandled errors in Slate could cause the editor to crash or exhibit unexpected behavior (as evidenced by the numerous linked issues). Implementing | undefined and conditional checks improves code stability and reduces the likelihood of crashes, leading to a more reliable and stable user experience.
3. Simplified debugging: The introduction of | undefined encourages developers to consciously address potential errors in their code. By being aware of possible issues and handling them explicitly, developers can more easily identify and resolve bugs in their codebase. This approach contributes to better overall code quality and maintainability.

While I acknowledge your concerns, I believe that the benefits of this approach outweigh the drawbacks.

As for the community feedback: #3641 shows 57 users in favor of OP versus (less than) 3 users, but I'd be curious to hear from those who could not give their feedback yet.

Your idea of wrapping Slate queries/transforms in error handling logic is an interesting alternative and could provide a helpful solution for developers who prefer to maintain their current error handling behavior without migrating to the new | undefined return types. As a way to move forward with the improvements mentioned above, we could also consider wrapping the new behavior so that all Slate queries do not return | undefined (could be just a type change). Silent errors would be the new default but could be reverted to error throwing by just overriding onError as mentioned in the changeset. What do you think?

Something like:

// no need to throw error here, it's the responsibility of `onError`
export const toDOMPoint = (editor: ReactEditor, point: Point) => {
  return ReactEditor.toDOMPoint(editor, point) as DOMPoint;
};

// or non-null assertion to all calls
toDOMPoint(editor, point)!

[BrentFarese]

@zbeyens I think in general you are right that letting developers figure out how to handle the errors is better. I can review the code in more detail later. I think in general we just want to ensure onError or whatever handler is passed the "errors" Slate used to throw, and if we're doing that then at least we're not hiding anything/silently failing. The errors are still discoverable.

[zbeyens]

@BrentFarese Sounds good, every single throw has been replaced by onError so we should be good on that point. I've also added a few onError calls to apply – this pattern allows us to add much more logging without affecting the user experience.

---

Thanks to @12joan, I have updated ErrorLogger to be a class supporting multiple error handlers – description/doc/changeset updated.

[zbeyens]

Based on the feedback we've received, we are leaning towards the idea that onError should throw by default, allowing the developer to override this behavior if needed.

This approach should reduce the risk of silent failures that might result in a corrupted state and potential data loss, which is a key concern. Besides that, we agreed on:

• the overridable onError function, so that developers can provide their own error handling mechanisms if desired.
• returning undefined instead of throwing exceptions in Slate API.
• moving the onError handling from the Slate API to the core of Slate. That way, the error handling responsibility is on the consumer side, not the API itself. For instance, if a function like Node.parent cannot find a parent, it should return undefined, and the method caller should handle that edge case. This makes the API more intuitive for the end user.

I'll try to find some time in the coming weeks to proceed.

[dscape]

As an app developer using Slate, merging this pull request has no impact on our app and requires no migration. However, Slate lacks error handling, leading to user confusion and delays in addressing even minor issues.

This pull request is crucial for managing Slate and providing better error handling to users. We're considering using it as a patch due to Slate's current limitations in a production environment.

Kudos to @zbeyens for the valuable contribution.

[zbeyens]

Here is my last update, still open to feedback.

• We're keeping the current behavior as the default until the stability of the new one is assured (editor.strict = true).
• You can opt for the new behavior (error-free editor) by setting editor.strict = false or overriding editor.onError default behavior.

Here's the updated types and method:

export type EditorError<T extends SlateErrorType = SlateErrorType> = {
  key: string       // Unique key: `<methodName>.<case>`
  message: string   // Contextual error description
  error?: SlateError<T>   // Underlying generic error
  data?: unknown    // Additional operation data
  recovery?: unknown   // Recovery value to return on error.
}

export type SlateError<T extends SlateErrorType = SlateErrorType> = {
  type: T          // Error type
  message: string  // Error description
}

export const onError = <T extends SlateErrorType>(
  editor: Editor,
  context: EditorError
): any => {
  const { message, recovery } = context

  if (editor.strict) throw new Error(message)
  editor.errors.push(context)
  return recovery
}

Key features include:

• A unique key for target specific errors.
• Experimental: to return something else than undefined, Slate defines a recovery value to return for many cases. For example, isEnd has recovery: false, but you could override it by returning another value. This in combination with data should give a lot of flexibility for quick fixes.
• Removal of the global error handler. Methods not dependent on the editor will return undefined or continue the for loop, which could lead to better or unexpected behaviors. Editor methods will now call onError for each undefined case.

[skogsmaskin]

I want to kudos you @zbeyens for this work too! The impossibility to catch slate-react's toDOMPoint errors has been such a pain point for a very long time making it super hard to write a realtime collaborative applications using (especially) slate-react. slate in it self has been less problematic with this.

So looking forward to getting this merged. I would approve it as it is now, as @dscape said above the changes are non-breaking and adds a ton of value already.

BTW: I'm wondering if we should start using maturity indicators in the codebase (like tag them @public, @beta, @alpha tags), especially with the API suface. This way new features (like this) could be merged earlier, and get some real world usage and experience before finally moving them into @public at some point.

[dscape]

@ianstormtaylor any updates on this?

[dylans]

@ianstormtaylor any updates on this?

I was away for a couple of weeks on holiday, but I will review this asap. Ian isn't currently active, so it's pretty much up to me to make sure this addresses the earlier concerns that were raised (there were conversations on Slack that led to the recent changes).

[dscape]

Thanks @dylans appreciate it!

[dscape]

@dylan Can you give an update?

Three months to merge such an important piece of work, why?

[gdehmlow]

+1 on this. For production error logging, we don't want to leak potential PII from the editor content in the error message, and having to scrub this information at our top level error logger is a pain and error prone. Thank you for this work!!

[dylans]

@dylan Can you give an update?

Three months to merge such an important piece of work, why?

Slate is used by a lot of people and big changes take time to think through.

We had issues with the first version (felt it was squelching errors rather than actually solving the issue). I'm going to give it one more review and land it soon.

@zbeyens there's a small merge conflict to resolve after the recent range selection fix for Firefox.

[dscape]

@dylan Can you give an update?
Three months to merge such an important piece of work, why?

Slate is used by a lot of people and big changes take time to think through.

We had issues with the first version (felt it was squelching errors rather than actually solving the issue). I'm going to give it one more review and land it soon.

@zbeyens there's a small merge conflict to resolve after the recent range selection fix for Firefox.

Appreciate it, open source work is ungrateful. I think everyone appreciates your work, and Ziad's, on slate.

Also Slate is indeed used by lots of people. This is why this change will be so good for everyone. Speaking about Decipad, these sort of "uncatchable" errors account for over 50% of the errors we don't (and can't) handle. While this change will undoubtfuly be hard to merge, as a user i accept that this risk is actually "a good thing".

Seems like others chipping in agree.

If there's something we can do to support this, let us know

[dylans]

Ok, so I am definitely struggling with this one for a few reasons:

It's several types of changes intermixed, leading to a high chance of breaking everyone.

I am reminded of the comments in #3641 (comment) and #3641 (comment)

In our app, we simply place an ErrorBoundary around the render function. So for example, if you have the Cannot resolve a DOM point from a Slate point issue, e.g. at https://codesandbox.io/s/inspiring-tristan-zq8lj4?file=/src/App.tsx

You might do an ErrorBoundary like this (ignore some of the older React patterns, just following the CodeSandbox example):

<ErrorBoundary editor={editor}>
        <Slate editor={editor} value={value} onChange={onChange}>
          <Editable onDOMBeforeInput={onBeforeInput} />
        </Slate>
      </ErrorBoundary>

which would be implemented like this:

import { Editable, Slate, withReact } from "slate-react";
import { Node, createEditor, Transforms } from "slate";
import React, { useCallback, useMemo, useState } from "react";
import { withHistory } from "slate-history";
class ErrorBoundary extends React.Component {
  static getDerivedStateFromError(error) {
    return {};
  }
  componentDidCatch(error, info) {
    // if selection issue then
    Transforms.deselect(this.props.editor);
  }
  render() {
    return this.props.children;
  }
}

That said, this still wouldn't let you fully recover from the error per se. So how do you recover from the state getting corrupted when you run into an error? ( I'm not sure the recovery attempt in this PR actually works).

As far as I can tell this doesn’t actually recover from errors, but might eat exceptions or stop the editor and leave it in a random state that cannot easily be recovered from, especially if you then allow edits to continue.

I would recommend that we start small... maybe we can start by relaxing toDomPoint or write better tests to reduce the number of scenarios where it breaks. So far example if deselect fails you might call:

Transforms.select(this.props.editor, {
      anchor: { path: [0, 0], offset: 0 },
      focus: { path: [0, 0], offset: 0 }
    });

Since you have no way to recover anyway.

So what I keep asking myself is what does this PR achieve that an ErrorBoundary does not achieve?

[zbeyens]

The new onError approach seeks to provide a granular, customizable error-handling API that extends beyond what the ErrorBoundary approach can provide. While an ErrorBoundary indeed helps capture generic errors, it doesn't provide enough context for handling that error.

In contrast, the onError handler provides detailed information about the error context, including the unique error key, underlying error type, and additional operation data. This granular information allows developers to create targeted error recovery strategies that can help maintain a stable editor.

Moreover, the onError approach provides an optional recovery value (experimental!) to facilitate an immediate fix for many cases, such as isEnd returning false. This combined with the data attribute should offer enough flexibility for quick fixes without compromising the editor state. Then I expect many people patching slate from the fixes they could write thanks to the new API.

Remember, we want to keep the old behavior as the default until we are sure the new one works well (editor.strict = true). So there is no "high chance of breaking everyone" by default. This is surely a more dangerous strategy than crashing the editor and/or resetting its state, this is why we actually need this so we can make it less dangerous with the incoming patches.

As for your suggestion to fix the actual slate issues so that we don't need to handle these errors, this is a more tedious task that requires much more resources.

I'm happy to hear more feedback and suggestions.

[pgte]

As the CTO and developer of a product that has been using Slate for more than 2 years, I can say that this fix would improve the overall robustness of our code. This would allow us to recover from commonly seen errors where, right now, we can only react to by resetting the state, which can be an awful user experience for larger documents.

[dscape]

This is how it feels to be a slatejs developer without error handling.

Screen.Recording.2023-08-10.at.14.00.41.mov

Lot's of users affected, incredibly limited options to tackle it. Most of the bugs are between browser support (and quirks), React, and Slate. Potentially also a plugin ecosystem like Plate. For instance, there's still some lack of standard about triple clicks handling in modern browsers. If there's an inconsistency between a browser and slate and slate has a bug, you won't be able to support your user (this is a real bug that was recently patched in slate)

Let's say you have a editor and do what @dylans suggested: You set an error boundary, but it lacks context. Because we don't know full cause, all we can do is re-render and check that we don't get into a loop. Eventually we need to tell the user to refresh the page.

From a user perspective, they see a "strange behaviour and re-render" and then an error page. "Ups, its broken" -- You will loose users, and there's little you can do about that.

If we had error handling we could, as app developers, say: if its a selection error let's not crash the editor.

These behaviours are specific to the application, so they should be the application developer responsibility. With slate this is just not possible.

I have sympathy and kudos for the maintainers of slate. It's a very hard thing to maintain and create. It builds on between a very old technology in html and things that often aren't even standard, like triple clicks. In between there's an expectation that the browser must support editing similar to native apps, or better. This includes voids vs. non voids, cell re-renders, and lot's of complexity to maintain. In our case using CRDTs on top. We know this is hard, we know it goes across lot's of layers.

But imo not having error handling and letting applications be at the mercy of the bugs is not acceptable. It just doesn't allow us to respond to people using the tech, and therefor should be a big issue for people when they choose slate over an alternative.While I'm sure that this PR could be improved, and more will be learned once it is merged and it will evolve, i don't understand why this is being kept at bay.

@zbeyens did high quality work here, and is open to feedback and recommendations. What else can be achieved here?

In the meantime, at Decipad we are considering having to maintain this as a patch on top of slate so we can support our users. I hope the maintainers reconsider their position here, or offer an alternative that empowers Slate users.

[dylans]

@zbeyens and I are good collaborators and have worked together on Plate for a long time.

For something as big and widely used as Slate, I strive to err on the side of the smallest most incremental changes possible so that we can revert quickly if necessary. This is a big change that would not be easy to reverse in its current form.

Fundamentally what makes me uncomfortable is multiple types of changes on different levels via one large PR, and slate-react still doesn't have a good test library which makes me more conservative than I would like to be.

For example, there are some fixes in the PR that likely fix a few errors or make it clearer why something is an error. I would suggest a PR first for those and land those.

The change to | undefined on a lot of things is a pretty big change. I would do that as a separate PR. Having a decent test library for slate-react would make me happier as well.

Then I would sort of ask, could we make better use of throw new Error vs. onError to provide additional context as an intermediary step. I'm not opposed to onError, just wondering how much we can get out of the existing paradigms first.

It's clear that some can get more out of ErrorBoundary ( https://github.com/bugbakery/transcribee/pull/337/files ) at least for selection errors.

Where I remain skeptical is error recovery rather than fixing things that cause errors, but I have an open mind and am not opposed to it. But I am a lot more comfortable getting there incrementally, whether it's as proposed above or in some other form.

[zbeyens]

I can understand this PR has a few risks we could optimize from Dylan’s review. I won’t have free time to split that PR for a few weeks so my recommendation is to either do a test release (Slate has a workflow to do that through a comment) so a few consumers could test it and give their feedback for the stable release. Or someone else could voluntarily split that PR.

[dscape]

I don't have the tech skill to support this split, but at Decipad we would be happy to support a bounty if it helps the project.

@dylans appreciate the response. As someone that used to be involved and maintain a lot of open source projects I hope you understand we are not, in any way, not recognising the work. Just trying to raise the awareness of the issue, as your day to day using slate might be different from ours.

[beorn]

Any update on this? It would be great to have a way to catch errors in Slate.

[zbeyens]

We need to split this PR. Since it's no longer my priority, anyone is welcome to take care of it.