Slate throws exceptions too liberally in relation to selection failures

[mpkelly]

Hello,

I would like to start a discussion about error handling in Slate which I think is too strict as Errors are often thrown for things that could just as easily be ignored or replaced by a warning. These are in functions like toDOMPoint or toSlateRange when Slate is trying to do things like map a HTMLElement to a Node but cannot because there is no direct mapping (the HTMLElement is created indirectly by a Slate Node). In many cases Slate will throw an Error that will take down the whole page. I understand that various attributes can be applied to have Slate ignore certain regions of the editor but I've still found it a bit of a fight and I really don't understand why Slate can't just return early in cases where no DOM-to-Slate mapping exists. In fact, I have forked Slate and disabled these Errors (here and here) and made Slate return early and while this fork is a WIP, I can tell you that my app is much better for it - it's less work for me and I will still find problems where things aren't wired-up correctly and I no longer have anxiety of deploying to production and worrying about the WSOD when the user's only crime was clicking on an empty <div/> (I know prod apps should have error handling but again it feels like I am having to handle more errors than is really necessary).

Some ideas:

• As above, log a warning like React does when in development mode and only use Error when it is absolutely necessary (cannot be recovered/ignored).
• Maybe Slate could add a onError callback to its API for users to learn about errors

Thoughts?

[cameracker]

Perhaps some error handling could be relaxed, but I personally don't have a strong opinion.

However, I can't help but wonder why people aren't catching the errors and letting the page crash? https://reactjs.org/blog/2017/07/26/error-handling-in-react-16.html

[mpkelly]

Hi @CameronAckermanSEL, I use componentDidCatch in production as it is necessary whether using Slate or not. However, it feels like Slate is the source of too many errors and that's the key point. There is also the development experience when first getting started to consider, too - here people might not have proper error handling in place and have to constantly restart/refresh/whatever due to errors for things they probably don't even care about.

Slate seems to think it has to do something based on every interaction with the editor otherwise there is a bug but I don't think this is right. I guess raising errors was just the easiest approach for the library devs when trying to get 0.50 ready but I don't think this is the best longterm approach for the library consumers who should ideally decide what is critical and what is not.

[skogsmaskin]

So far the only error that have been really problematic for me is this one (Cannot resolve a DOM point from Slate point):

Tracked in own issue here: https://github.com/ianstormtaylor/slate/issues/3575 (my comment here https://github.com/ianstormtaylor/slate/issues/3575#issuecomment-620568898)

It makes collaborative editing near impossible, because as far as I can tell, there is no way we are able to adjust the selection when a new value is coming in through props before slate-react tries to naively restore the current user selection to a potential invalid DOM-range.

[gabriel-peracio]

@skogsmaskin I've been working around it by storing the selection, then doing

 const contenteditableElement: any = window
      .getSelection()
      ?.anchorNode?.parentElement?.closest("[contenteditable]").blur();

then changing the value, and afterwards restoring the selection

I am not suggesting this horrible hack as a viable workaround. I have been using this because I am super annoyed at this bug and wanted to move on to something else while still avoiding the crashes.

[karlludwigweise]

A tale from a developers first experience with slate:

We researched a buch of richtext editors and decided that slate would make the best fit for our prupose (react-admin and a custom markdown editor). We followed the installation guide and the first thing that hit us was the very error message @skogsmaskin erorted as #3575.

I've seen a fix in a fork from @mpkelly but that's no long term fix.

My 2 cents are: When slate throws an error, it should not take the application down with it. And for the specific error #3575 a smart default would fix most cases.

[mpkelly]

@karlludwigweise my fork wasn't intended to fix #3575 per se but only stop Slate from raising errors for things my app does not care about, like clicking on some structural HTML elements. I still think Slate should be changed to return Node | null rather than just returning Node and raising an Error if it can't do so. It should be the library consumers job to decide what is critical and what is not IMHO.

Should also point out that my fork is a quick and dirty fix done with a simple code search. It was not intended to be a community fix and I only mentioned it above to make it clear which parts of the Slate codebase I think should be changed.

[cameracker]

My 2 cents are: When slate throws an error, it should not take the application down with it. And for the specific error #3575 a smart default would fix most cases.

Still need to reiterate that exceptions are catchable via react lifecycle events. It is possible for these selection problems to be caught and dealt with. I hate to be blunt here, but in my opinon if the developer is allowing slate to crash their application, then that's on the developer. I also suspect that many of these errors can be pre-empted with checks before applying an OT in the case of collaborative editing. But unfortunately, there aren't a lot of provided reproductions of the problem.

I have a few things I want to respond to and points I want to make:

1. "clicking on some structural HTML elements" This, imo, is an (undocumented) user error. If an element is structural and should not be selected because it's not modeled in the slate value, it is up to the developer to make sure this element can't be selected in the dom. This can be done by using css user-select: none or using contentEditable=false. Alternatively, structural elements can be represented in the slate value to circumvent this issue. I would expect an exception to be thrown in this case.

2. Slate is throwing these exceptions because a lot of the time selection is used to determine where a transformation should be applied in the core features. If selection does not point to a valid node, then slate can't perform a transformation so it bails. This seems to me like a reasonable control flow to throw an exception. I worry that having a first class handler to move selection somewhere as a recovery will break features in subtle ways that will cause more harm than good. Have not done a meaningful amount of evaluation into what specific transforms would be impacted by automatically moving selection.

3. This issue is very broad because there are a lot of cases where an exception for invalid selection might be thrown. It might be thrown because a selected node is gone because of an external OT. It might be thrown because the user has allowed a node to be selected on the DOM that has no corresponding slate value. That this issue is being treated as a grab all for every selection exception, makes this problem very difficult to talk about because it precludes precise discussion about whether these exceptions are from actual bugs, user error, and it makes it hard to keep track of work arounds.

4. I don't see many concrete proposals. I get its frustrating, but at this time there aren't a lot of strategies documented that we could apply to the library to alleviate this UX issue, and there doesn't seem to be a lot of information we could use to write user guides. At the moment, this feels more like a complaint thread and I think we can do better than that.

5. There is a general suggestion that slate should have a reasonable default for what selection should move to, but I feel like people are underestimating how domain specific selection can be. A major goal of slate is to he highly customizable, and have a minimal set of opinions it enforces in the model. In many cases, this goal precludes defaults (such as default block types, and here default fallback selections) because it can't guess what the domain rules of the user's implementation are. At some level, this is by design. Again, a more fleshed out suggestion on where slate can or should move selection would be nice.

I am thinking that it's probably better in the long run to close this issue and lock discussion in favor of splitting this up into more specific instances of this problem. Interested in people's thoughts. Thanks!

[cameracker]

Current categories of selection failures (from this thread):

1. The DOM selection is on a node that is not represented in the slate value

   • IMO, this is a user error and can be alleviated with making the node have a block in the slate value, or by making the node unselectable in the DOM (via contentEditable=false or user-select:none). We can mitigate this with more documentation or a FAQ.

2. An external event removes a node that is selected by slate

   • I've seen this mentioned when a user is dealing with OT, but it's unclear whether this user was attempting to check whether the node about to be removed was selected prior to applying the operation. Slate could arguably handle this case, but the location where selection should move is likely very situational and hard to bake into core as a default.

3. An external event changes a node that is selected by slate

   • Mentioned by a user in slack who was mutating the value directly without the editor controller, and injecting the value back in via setValue. This probably happened because the node that was selected had an address change because immer, but unsure why the path based selection throws.
   • The same user said they solved this problem by checking whether the selected node has been mutated before applying the value. This seems like a good work around. Is this a viable solution?

Suggestions:

1. Provide support for a user defined callback for responding to selection failures, this will allow a user to move the selection.

   • Does the operation resume after the selection is restored? Where can this move cause bugs based on slate moving selection after validation but before making changes to state?

2. Have a default selection that slate moves to if it fails

   • Fallback selection is likely dependent on the users domain rules and may be difficult to enforce a reasonable default. We would likely also need a reasonable override for such a default selection feature, but over all defaults like this may go against some of slate's "highly customizable, unopinionated" values. But is probably possible somehow

3. Have slate emit a warning and stop whatever it was doing when selection failed, or silently fail

   • this matches how a lot of users have been working around the issue, but I think this would violate a lot of usage expectations if this pattern were to be applied everywhere and it might lead to more complicated bugs that can't be worked around easily.

[antondc]

I was developing an editor with Slate and React, but found several situations where I can not avoid the errors thrown by it: for example, when using Katex library to render math formulas.

Of course we can add an error boundary to the slate-react Editable component; but when a Cannot resolve a DOM point from Slate point error is thrown the Editable component will crash anyway, and the whole flow is lost.

Just wonder if there was some work done in the direction mentioned by @mpkelly and @cameracker

[dylans]

I was developing an editor with Slate and React, but found several situations where I can not avoid the errors thrown by it: for example, when using Katex library to render math formulas.

Of course we can add an error boundary to the slate-react Editable component; but when a Cannot resolve a DOM point from Slate point error is thrown the Editable component will crash anyway, and the whole flow is lost.

Just wonder if there was some work done in the direction mentioned by @mpkelly and @cameracker

I don't believe much has progressed in this area, though the topic does come up pretty regularly, and along with things like #4769 . It's an area I too would like to see improve this year.

[antondc]

Thanks for the response @dylans. I'll we looking forward for it: in its current implementation we can not go to production with slate due to this issue.

[Nikhil22]

May not apply to your specific use-case, but I found a workaround detailed in #5229 , in my comment.

[bryanph]

@antondc I've been using katex for math rendering and haven't run into the issues you mentioned. If you can post a reproducible example on codepen or something that would be useful.

I do think the error could be more detailed sometimes and provide a path of action for resolving it.

[zbeyens]

Discussed with @12joan on that one. We've ended up on a design that uses a separate "staging" editor that is not rendered:

• apply any incoming operation to the staging editor first.
• if it does not throw, it can be safely applied to the editor.

The goal is to have a system that prevents:

• normalizer errors
• any operation errors
• any transforms/queries (e.g. toDOMPoint) errors
• bonus with plate: log/disable the faulty plugin calling that operation

The alternative would have been to not use a staging editor and "rollback" faulty operations but I'm not comfortable with implementing that design.

As for the error throwing debate, there are many opinions:

1. keep as it is
2. early return null on "error"
3. console.warn the "errors"

For functions having editor as parameter, the common solution would be an editor method onError so we can pick one of the above behavior. For functions not having access to editor, we can't please everyone. We'll either need to pass a new option onError on all of these methods, choose one of the 3 above behaviors or reuse the Scrubber design https://github.com/ianstormtaylor/slate/pull/4999.

PR is WIP.

[HugoDerigny]

@antondc I've been using katex for math rendering and haven't run into the issues you mentioned. If you can post a reproducible example on codepen or something that would be useful.

I do think the error could be more detailed sometimes and provide a path of action for resolving it.

Could you share an example on how you use Katex with the Editor please ?

[zbeyens]

Addressed in https://github.com/ianstormtaylor/slate/pull/5407