ianstormtaylor / slate

A completely customizable framework for building rich text editors. (Currently in beta.)
http://slatejs.org
MIT License
29.62k stars 3.23k forks source link

Proposal: Improve the reliability of `ReactEditor.findPath` without compromising its efficiency #5697

Open 12joan opened 3 weeks ago

12joan commented 3 weeks ago

Problem It looks to me like the only reason findPath is coupled to slate-react is for performance reasons. By using slate-react’s NODE_TO_PARENT and NODE_TO_INDEX weak maps, ReactEditor.findPath is able to find the path of any rendered node very efficiently, in linear time relative to the depth of the node to be found.

However, this comes with some drawbacks:

Solution My proposed solution has two parts.

First, we introduce a new Editor.findPath method in the core Slate package, which finds the path of a node by traversing the entire tree using Editor.nodes until a matching node is found.

Second, we improve the reliability of ReactEditor.findPath by having it validate the information from the weak maps against the actual editor value, and have it use Editor.findPath as a fallback instead of throwing an error.

Algorithm for ReactEditor.findPath:

  1. Get the parent and index for child from the weak maps
  2. New: Validate that parent.children[index] === child
  3. New: If the above validation fails, check if parent.children.indexOf(child) finds a matching node. If so, replace index with this new index and continue. Otherwise, throw an error.
  4. Prepend index to the path and set child equal to parent
  5. Repeat steps 1-4 until we reach the editor node
  6. New: If the above algorithm throws an error, fall back to Editor.findPath, which will throw its own error if the node genuinely doesn't exist

Alternatives If we decide that the performance benefit from using the weak maps-based solution isn't worth the drawbacks or added compexity to work around those drawbacks, we could remove it entirely and replace it with the traversal-based solution.

However, I think the performance benefit from using weak maps is justified. I've seen many apps call ReactEditor.findPath at the top level of a React component (although admittedly this is something I tend to discourage). In an app where all nodes do this, using a traversal-based solution by default would increase the complexity of rendering all nodes from O(N) to O(N^2), where N is the number of nodes in the editor.

I think using weak maps by default but using traversal as a fallback is the best solution.

Context In Plate, we’re trying to decouple our plugins from React. We’ve found that the dependency of the findPath function on slate-react is an obstacle to this.

We're also considering adding an editor.findPath method that defaults to Editor.findPath but which withReact automatically replaces with ReactEditor.findPath. The issue with this is that the pattern established in #5307 has methods in Editor depending on editor, not the other way around. For that reason, I think we'll probably do this in Plate only rather than in Slate.

WindrunnerMax commented 3 weeks ago

Regarding the issues encountered with using ReactEditor.findPath, I also have some thoughts and solutions:

  1. I will use ReactEditor.findPath as the primary method. If it fails, I will iterate over editor.children as a fallback to ensure correct data retrieval as much as possible.
  2. After using Transforms to change the content, I won't immediately get the path value. Instead, I'll wait until React finishes rendering before proceeding to the next step. This way, I can execute the relevant operations sequentially. Thanks to the lack of additional asynchronous operations in Slate, I can easily determine when the current <Editable /> rendering is complete within useEffect.
12joan commented 3 weeks ago

@WindrunnerMax Those are good workarounds. I'm hoping that with the changes to ReactEditor.findPath I describe above, those workaround won't be necessary anymore.