Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
12joan opened this issue Aug 19, 2024 · 2 comments

Comments

@12joan
Copy link
Contributor

12joan commented Aug 19, 2024

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:

  • It can only be used in code that's coupled to React, which means it can't be used on the server
  • It will throw an error if the node to be found has only just been inserted
  • It will return an incorrect path if the path of the node to be found has just been changed by an operation
  • It presents an obstacle to supporting conditionally rendered nodes in slate-react, which is a feature I'd like to see implemented one day

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
Copy link
Contributor

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
Copy link
Contributor Author

12joan commented Aug 20, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants