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

feat: implement findPath without relying on DOM #5800

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

yf-yang
Copy link
Contributor

@yf-yang yf-yang commented Jan 27, 2025

Current Status
I call editor.findPath from DOMEditor.findPath and compare the new implementation's returned path with legacy implementation's returned path.

Currently all tests can pass and original calls are not migrated yet. Need to discuss what kind of test cases are needed.

Description
This PR aims at replacing ReactEditor.findPath with Editor.findPath

Currently, two weakmaps NODE_TO_PARENT and NODE_TO_INDEX are set when rendering the React component tree. These two weakmaps help compute a node's path.

However, if we are going to support virtualization, then not all components will be rendered, which means some nodes' information cannot be collected. Therefore, we'll not rely on React to compute the path of each element.

In this PR, we create an entry editor._caches which holds internal caches. Currently it only contains nodeToParent, which works exactly the same like NODE_TO_PARENT before.

We patch normalizeNode function so that when a node becomes dirty, then we update all of its children's parent to this new node.

NODE_TO_INDEX no longer exists, we'll directly call parent.children.indexOf. Since it is a native API, we assume it is fast enough.

When finding a path, it would find parents until root (editor), then get all levels of indices, then construct the whole path.

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

Copy link

changeset-bot bot commented Jan 27, 2025

🦋 Changeset detected

Latest commit: f5e771f

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

This PR includes changesets to release 2 packages
Name Type
slate-dom Minor
slate 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

Comment on lines +14 to +20
if (!parent) {
throw new Error(
`Unable to find the path for Slate node (parent not found): ${Scrubber.stringify(
node
)}`
)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could occur during normal use if editor.children is modified in a way that bypasses normaliseNode, such as when setting editor.children directly or when finding the path of a newly added node inside withoutNormalizing. I'd suggest traversing the full tree to repair the cache in this case, and only throw an error if the parent still cannot be found. In addition, the withoutNormalizing case can be mitigated by updating the cache in editor.apply instead of normalizeNode.

Comment on lines +26 to +30
throw new Error(
`Unable to find the path for Slate node (node is not child of its parent): ${Scrubber.stringify(
node
)}`
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar situation to above

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

Successfully merging this pull request may close these issues.

2 participants