fix: HistoryEditor helpers leaving flags stuck when fn throws, and nested withNewBatch losing the enclosing batch's split

.changeset/silver-scissors-attack.md

@@ -0,0 +1,5 @@
+---
+'slate-history': patch
+---
+
+Fix `HistoryEditor.withMerging`, `HistoryEditor.withNewBatch` and `HistoryEditor.withoutMerging` leaving the history flags corrupted when `fn` throws, and a nested `withNewBatch` that applies no operation clearing the pending split of the enclosing `withNewBatch`.

packages/slate-history/src/history-editor.ts

@@ -82,8 +82,11 @@ export const HistoryEditor = {
   withMerging(editor: HistoryEditor, fn: () => void): void {
     const prev = HistoryEditor.isMerging(editor)
     MERGING.set(editor, true)
-    fn()
-    MERGING.set(editor, prev)
+    try {
+      fn()
+    } finally {
+      MERGING.set(editor, prev)
+    }
   },
 
   /**
@@ -92,12 +95,18 @@ export const HistoryEditor = {
    * merged as usual.
    */
   withNewBatch(editor: HistoryEditor, fn: () => void): void {
-    const prev = HistoryEditor.isMerging(editor)
+    const prevMerging = HistoryEditor.isMerging(editor)
+    const prevSplitting = HistoryEditor.isSplittingOnce(editor)
     MERGING.set(editor, true)
     SPLITTING_ONCE.set(editor, true)
-    fn()
-    MERGING.set(editor, prev)
-    SPLITTING_ONCE.delete(editor)
+    try {
+      fn()
+    } finally {
+      MERGING.set(editor, prevMerging)
+      if (!prevSplitting) {
+        SPLITTING_ONCE.delete(editor)
+      }
+    }
   },
 
   /**
@@ -108,8 +117,11 @@ export const HistoryEditor = {
   withoutMerging(editor: HistoryEditor, fn: () => void): void {
     const prev = HistoryEditor.isMerging(editor)
     MERGING.set(editor, false)
-    fn()
-    MERGING.set(editor, prev)
+    try {
+      fn()
+    } finally {
+      MERGING.set(editor, prev)
+    }
   },
 
   /**

packages/slate-history/test/history-editor.ts

@@ -0,0 +1,42 @@
+import assert from 'assert'
+import { createEditor } from 'slate'
+import { HistoryEditor, withHistory } from '..'
+
+const boom = () => {
+  throw new Error('boom')
+}
+
+describe('HistoryEditor', () => {
+  describe('withMerging', () => {
+    it('restores the merging flag when fn throws', () => {
+      const editor = withHistory(createEditor())
+      assert.throws(() => HistoryEditor.withMerging(editor, boom), /boom/)
+      assert.strictEqual(HistoryEditor.isMerging(editor), undefined)
+    })
+  })
+
+  describe('withNewBatch', () => {
+    it('restores the merging and splitting flags when fn throws', () => {
+      const editor = withHistory(createEditor())
+      assert.throws(() => HistoryEditor.withNewBatch(editor, boom), /boom/)
+      assert.strictEqual(HistoryEditor.isMerging(editor), undefined)
+      assert.strictEqual(HistoryEditor.isSplittingOnce(editor), undefined)
+    })
+  })
+
+  describe('withoutMerging', () => {
+    it('restores the merging flag when fn throws', () => {
+      const editor = withHistory(createEditor())
+      assert.throws(() => HistoryEditor.withoutMerging(editor, boom), /boom/)
+      assert.strictEqual(HistoryEditor.isMerging(editor), undefined)
+    })
+  })
+
+  describe('withoutSaving', () => {
+    it('restores the saving flag when fn throws', () => {
+      const editor = withHistory(createEditor())
+      assert.throws(() => HistoryEditor.withoutSaving(editor, boom), /boom/)
+      assert.strictEqual(HistoryEditor.isSaving(editor), undefined)
+    })
+  })
+})

packages/slate-history/test/undo/with_new_batch/nested-empty.tsx

@@ -0,0 +1,27 @@
+/** @jsx jsx */
+import { jsx } from '../..'
+import { HistoryEditor } from '../../..'
+
+export const run = editor => {
+  editor.insertText('x')
+  HistoryEditor.withNewBatch(editor, () => {
+    HistoryEditor.withNewBatch(editor, () => {})
+    editor.insertText('y')
+  })
+}
+export const input = (
+  <editor>
+    <block>
+      one
+      <cursor />
+    </block>
+  </editor>
+)
+export const output = (
+  <editor>
+    <block>
+      onex
+      <cursor />
+    </block>
+  </editor>
+)

packages/slate-history/test/undo/with_new_batch/nested-throws.tsx

@@ -0,0 +1,32 @@
+/** @jsx jsx */
+import assert from 'assert'
+import { jsx } from '../..'
+import { HistoryEditor } from '../../..'
+
+export const run = editor => {
+  editor.insertText('x')
+  HistoryEditor.withNewBatch(editor, () => {
+    assert.throws(() => {
+      HistoryEditor.withNewBatch(editor, () => {
+        throw new Error('boom')
+      })
+    }, /boom/)
+    editor.insertText('y')
+  })
+}
+export const input = (
+  <editor>
+    <block>
+      one
+      <cursor />
+    </block>
+  </editor>
+)
+export const output = (
+  <editor>
+    <block>
+      onex
+      <cursor />
+    </block>
+  </editor>
+)

packages/slate-history/test/undo/with_new_batch/nested.tsx

@@ -0,0 +1,29 @@
+/** @jsx jsx */
+import { jsx } from '../..'
+import { HistoryEditor } from '../../..'
+
+export const run = editor => {
+  editor.insertText('x')
+  HistoryEditor.withNewBatch(editor, () => {
+    HistoryEditor.withNewBatch(editor, () => {
+      editor.insertText('a')
+    })
+    editor.insertText('b')
+  })
+}
+export const input = (
+  <editor>
+    <block>
+      one
+      <cursor />
+    </block>
+  </editor>
+)
+export const output = (
+  <editor>
+    <block>
+      onex
+      <cursor />
+    </block>
+  </editor>
+)

packages/slate-history/test/undo/with_new_batch/throws.tsx

@@ -0,0 +1,24 @@
+/** @jsx jsx */
+import assert from 'assert'
+import { jsx } from '../..'
+import { HistoryEditor } from '../../..'
+import { cloneDeep } from 'lodash'
+
+export const run = editor => {
+  editor.insertText('x')
+  assert.throws(() => {
+    HistoryEditor.withNewBatch(editor, () => {
+      throw new Error('boom')
+    })
+  }, /boom/)
+  editor.insertText('y')
+}
+export const input = (
+  <editor>
+    <block>
+      one
+      <cursor />
+    </block>
+  </editor>
+)
+export const output = cloneDeep(input)

packages/slate-history/test/undo/without_merging/throws.tsx

@@ -0,0 +1,24 @@
+/** @jsx jsx */
+import assert from 'assert'
+import { jsx } from '../..'
+import { HistoryEditor } from '../../..'
+import { cloneDeep } from 'lodash'
+
+export const run = editor => {
+  assert.throws(() => {
+    HistoryEditor.withoutMerging(editor, () => {
+      throw new Error('boom')
+    })
+  }, /boom/)
+  editor.insertText('a')
+  editor.insertText('b')
+}
+export const input = (
+  <editor>
+    <block>
+      one
+      <cursor />
+    </block>
+  </editor>
+)
+export const output = cloneDeep(input)