Fix NODE_TO_KEY correction for split_node and merge_node (ianstormtay…

…lor#4901)

* Fix NODE_TO_KEY correction for split_node and merge_node

* fix lint

* add changeset

* Add NODE_TO_KEY tests for number of mounts for split_node and merge_node

.changeset/funny-humans-whisper.md

@@ -0,0 +1,5 @@
+---
+'slate-react': patch
+---
+
+Fixes a bug where nodes remounted on split_node and merge_node

packages/slate-react/src/plugin/with-react.ts

@@ -60,42 +60,38 @@ export const withReact = <T extends Editor>(editor: T) => {
     }
   }
 
+  // This attempts to reset the NODE_TO_KEY entry to the correct value
+  // as apply() changes the object reference and hence invalidates the NODE_TO_KEY entry
   e.apply = (op: Operation) => {
     const matches: [Path, Key][] = []
 
     switch (op.type) {
       case 'insert_text':
       case 'remove_text':
-      case 'set_node': {
-        for (const [node, path] of Editor.levels(e, { at: op.path })) {
-          const key = ReactEditor.findKey(e, node)
-          matches.push([path, key])
-        }
-
+      case 'set_node':
+      case 'split_node': {
+        matches.push(...getMatches(e, op.path))
         break
       }
 
       case 'insert_node':
-      case 'remove_node':
-      case 'merge_node':
-      case 'split_node': {
-        for (const [node, path] of Editor.levels(e, {
-          at: Path.parent(op.path),
-        })) {
-          const key = ReactEditor.findKey(e, node)
-          matches.push([path, key])
-        }
+      case 'remove_node': {
+        matches.push(...getMatches(e, Path.parent(op.path)))
+        break
+      }
 
+      case 'merge_node': {
+        const prevPath = Path.previous(op.path)
+        matches.push(...getMatches(e, prevPath))
         break
       }
 
       case 'move_node': {
-        for (const [node, path] of Editor.levels(e, {
-          at: Path.common(Path.parent(op.path), Path.parent(op.newPath)),
-        })) {
-          const key = ReactEditor.findKey(e, node)
-          matches.push([path, key])
-        }
+        const commonPath = Path.common(
+          Path.parent(op.path),
+          Path.parent(op.newPath)
+        )
+        matches.push(...getMatches(e, commonPath))
         break
       }
     }
@@ -255,3 +251,12 @@ export const withReact = <T extends Editor>(editor: T) => {
 
   return e
 }
+
+const getMatches = (e: Editor, path: Path) => {
+  const matches: [Path, Key][] = []
+  for (const [n, p] of Editor.levels(e, { at: path })) {
+    const key = ReactEditor.findKey(e, n)
+    matches.push([p, key])
+  }
+  return matches
+}

packages/slate-react/test/index.spec.tsx

@@ -1,5 +1,12 @@
 import React from 'react'
-import { createEditor, NodeEntry, Range } from 'slate'
+import {
+  createEditor,
+  NodeEntry,
+  Node,
+  Range,
+  Element,
+  Transforms,
+} from 'slate'
 import { create, act, ReactTestRenderer } from 'react-test-renderer'
 import {
   Slate,
@@ -11,14 +18,14 @@ import {
   DefaultLeaf,
 } from '../src'
 
+const createNodeMock = () => ({
+  ownerDocument: global.document,
+  getRootNode: () => global.document,
+})
+
 describe('slate-react', () => {
   describe('Editable', () => {
     describe('decorate', () => {
-      const createNodeMock = () => ({
-        ownerDocument: global.document,
-        getRootNode: () => global.document,
-      })
-
       it('should be called on all nodes in document', () => {
         const editor = withReact(createEditor())
         const value = [{ type: 'block', children: [{ text: '' }] }]
@@ -172,5 +179,72 @@ describe('slate-react', () => {
         )
       })
     })
+
+    describe('NODE_TO_KEY logic', () => {
+      it('should not unmount the node that gets split on a split_node operation', async () => {
+        const editor = withReact(createEditor())
+        const value = [{ type: 'block', children: [{ text: 'test' }] }]
+        const mounts = jest.fn<void, [Element]>()
+
+        let el: ReactTestRenderer
+
+        act(() => {
+          el = create(
+            <Slate editor={editor} value={value} onChange={() => {}}>
+              <DefaultEditable
+                renderElement={({ element, children }) => {
+                  React.useEffect(() => mounts(element), [])
+
+                  return children
+                }}
+              />
+            </Slate>,
+            { createNodeMock }
+          )
+        })
+
+        // slate updates at next tick, so we need this to be async
+        await act(async () =>
+          Transforms.splitNodes(editor, { at: { path: [0, 0], offset: 2 } })
+        )
+
+        // 2 renders, one for the main element and one for the split element
+        expect(mounts).toHaveBeenCalledTimes(2)
+      })
+
+      it('should not unmount the node that gets merged into on a merge_node operation', async () => {
+        const editor = withReact(createEditor())
+        const value = [
+          { type: 'block', children: [{ text: 'te' }] },
+          { type: 'block', children: [{ text: 'st' }] },
+        ]
+        const mounts = jest.fn<void, [Element]>()
+
+        let el: ReactTestRenderer
+
+        act(() => {
+          el = create(
+            <Slate editor={editor} value={value} onChange={() => {}}>
+              <DefaultEditable
+                renderElement={({ element, children }) => {
+                  React.useEffect(() => mounts(element), [])
+
+                  return children
+                }}
+              />
+            </Slate>,
+            { createNodeMock }
+          )
+        })
+
+        // slate updates at next tick, so we need this to be async
+        await act(async () =>
+          Transforms.mergeNodes(editor, { at: { path: [0, 0], offset: 0 } })
+        )
+
+        // only 2 renders for the initial render
+        expect(mounts).toHaveBeenCalledTimes(2)
+      })
+    })
   })
 })