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(normalize-node): Adding children field to prevent erronous nodes from breaking notebooks. #5620

Merged
merged 2 commits into from
Apr 1, 2024

Conversation

JohnCosta27
Copy link
Contributor

@JohnCosta27 JohnCosta27 commented Mar 25, 2024

Description
Sometimes, when working with other tools such as Yjs, it so happens that we enter nodes into editor.children that slate cannot recover from. The most common case I come across is erronous nodes that are elements, and somewhere along the process the children field was dropped.

This could happen for various reasons, but IMO slate should be able to recover from them.

Example
Let's say you have the following:

editor.children = [
    {
      type: "table",
      children: [
        {
          type: "row",
          children: [{ text: "empty " }],
        },
        {
          type: "not_row",
        },
      ],
    }
  ]

Using editor.normalize won't touch the not_row element, but if you attempt editor.normalize({ force: true }), slate will throw an error, and rightfully so.

This at least prepares the node, making it acceptable by Element.isElement, and allowing normalizer plugins the user might make, to easily normalize away these nodes, because otherwise they will not be able to be normalized.

Disclaimer

I've used Slate for a while, and I know a fair bit about it. But this could have missed the mark.

The way in which I do this doesn't seem ideal to me. It seems like an anti-pattern. But using the Transforms will result in the same error being thrown, because we assume that this Node has a children field.

Any help would be greatly appreciated.

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 Mar 25, 2024

🦋 Changeset detected

Latest commit: 9c31b12

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

This PR includes changesets to release 1 package
Name Type
slate Patch

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

@JohnCosta27
Copy link
Contributor Author

The elementChild.children = [] is not great, and definitely isn't like other parts of the codebase. Does anyone have a better way?

@dylans
Copy link
Collaborator

dylans commented Mar 26, 2024

Thinking about this one, will have some thoughts soon.

@JohnCosta27
Copy link
Contributor Author

Thinking about this one, will have some thoughts soon.

Thank you, I'll be more than happy to put in the work for this one

@dylans
Copy link
Collaborator

dylans commented Apr 1, 2024

I don't have a better suggestion, so let's land it. If it breaks other things we'll revert quickly and revisit.

@hernansartorio
Copy link
Contributor

Isn't this handled by default? From https://docs.slatejs.org/concepts/11-normalizing:

If an element node does not contain any children, an empty text node will be added as its only child.

Making it an empty array as introduced here actually creates an incorrect state. I realized this after part of my app which relied on the original behavior broke:

Cannot use 'in' operator to search for 'children' in undefined.

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.

3 participants