Improve normalize suppression to make less verbose and safer (#1549)

* change normalization can be set with setOperationFlag, and changes can be executed in sequence with automatic suppression with guaranteed document normalization at the end.

* responded to developer feedback by renaming execute to withMutations (to mirror immutable JS), implemented tests, updated documentation

* fixed typos discovered in review.

* fixed missing normalize flag usages and added withMutations to the schemas guide

* responded to developer feedback

* fixed lint errors and cleaned up code

* readd missing tests

* getFlag now allows options to override the change flags

* removed normalize restoration asserts from unit tests

* unit test cleanup

docs/guides/schemas.md

@@ -112,3 +112,67 @@ This validation defines a very specific (honestly, useless) behavior, where if a
 When you need this level of specificity, using the `validateNode` property of the editor or plugins is handy.
 
 However, only use it when you absolutely have to. And when you do, you need to be aware of its performance. `validateNode` will be called **every time the node changes**, so it should be as performant as possible. That's why the example above returns early, so that the smallest amount of work is done as possible each time it is called.
+
+## Multi-step Normalizations
+
+Some normalizations will require multiple `change` function calls in order to complete. But after calling the first change function, the resulting document will be normalized, changing it out from under you. This can cause unintended behaviors.
+
+Consider the following validation function that merges adjacent text nodes together. 
+
+Note: This functionality is already correctly implemented in slate-core so you don't need to put it in yourself!
+
+```
+/**
+  * Merge adjacent text nodes.
+  *
+  * @type {Object}
+  */
+validateNode(node) {
+  if (node.object != 'block' && node.object != 'inline') return
+
+  const invalids = node.nodes
+    .map((child, i) => {
+      const next = node.nodes.get(i + 1)
+      if (child.object != 'text') return
+      if (!next || next.object != 'text') return
+      return next
+    })
+    .filter(Boolean)
+
+  if (!invalids.size) return
+
+  return (change) => {
+    // Reverse the list to handle consecutive merges, since the earlier nodes
+    // will always exist after each merge.
+    invalids.reverse().forEach((n) => {
+      change.mergeNodeByKey(n.key)
+    })
+  }
+}
+```
+
+There is actually a problem with this code. Because each `change` function call will cause nodes impacted by the mutation to be normalized, this can cause interruptions to carefully implemented sequences of `change` functions and may create performance problems or errors. The normalization logic in the above example will merge the last node in the invalids list together, but then it'll trigger another normalization and start over!
+
+How can we deal with this? Well, normalization can be suppressed temporarily for multiple `change` function calls by using the `change.withoutNormalization` function. `withoutNormalization` accepts a function that takes a `change` object as a parameter, and executes the function while suppressing normalization. Once the function is done executing, the entire document is then normalized to pick up any unnnormalized transformations and ensure your document is in a normalized state.
+
+The above validation function can then be written as below
+
+```
+/**
+  * Merge adjacent text nodes.
+  *
+  * @type {Object}
+  */
+validateNode(node) {
+  ... 
+  return (change) => {
+    change.withoutNormalization((c) => {
+      // Reverse the list to handle consecutive merges, since the earlier nodes
+      // will always exist after each merge.
+      invalids.reverse().forEach((n) => {
+        c.mergeNodeByKey(n.key)
+      })
+    });
+  }
+}
+```

docs/reference/slate/change.md

@@ -50,6 +50,33 @@ function onSomeEvent(event, change) {
 }
 ```
 
+### `withoutNormalization`
+`withoutNormalization(customChange: Function) => Change`
+
+This method calls the provided `customChange` function with the current instance of the `Change` object as the first argument. While `customChange` is executing, normalization is temporarily suppressed, but normalization will be executed once the `customChange` function completes execution.
+
+The purpose of `withoutNormalization` is to allow a sequence of change operations that should not be interrupted by normalization. For example:
+
+```js
+/**
+ * Only allow block nodes in documents.
+ *
+ * @type {Object}
+ */
+validateNode(node) {
+  if (node.object != 'document') return
+  const invalids = node.nodes.filter(n => n.object != 'block')
+  if (!invalids.size) return
+
+  return (change) => {
+    change.withoutNormalization((c) => {
+      invalids.forEach((child) => {
+        c.removeNodeByKey(child.key)
+      })
+    })
+  }
+}
+```
 
 ## Current Value Changes
 

packages/slate/src/changes/at-range.js

@@ -28,7 +28,7 @@ const Changes = {}
 Changes.addMarkAtRange = (change, range, mark, options = {}) => {
   if (range.isCollapsed) return
 
-  const { normalize = true } = options
+  const normalize = change.getFlag('normalize', options)
   const { value } = change
   const { document } = value
   const { startKey, startOffset, endKey, endOffset } = range
@@ -77,7 +77,7 @@ Changes.deleteAtRange = (change, range, options = {}) => {
   // when you undo a delete, the expanded selection will be retained.
   change.snapshotSelection()
 
-  const { normalize = true } = options
+  const normalize = change.getFlag('normalize', options)
   const { value } = change
   let { startKey, startOffset, endKey, endOffset } = range
   let { document } = value
@@ -350,7 +350,7 @@ Changes.deleteWordBackwardAtRange = (change, range, options) => {
  */
 
 Changes.deleteBackwardAtRange = (change, range, n = 1, options = {}) => {
-  const { normalize = true } = options
+  const normalize = change.getFlag('normalize', options)
   const { value } = change
   const { document } = value
   const { startKey, focusOffset } = range
@@ -536,7 +536,7 @@ Changes.deleteWordForwardAtRange = (change, range, options) => {
  */
 
 Changes.deleteForwardAtRange = (change, range, n = 1, options = {}) => {
-  const { normalize = true } = options
+  const normalize = change.getFlag('normalize', options)
   const { value } = change
   const { document } = value
   const { startKey, focusOffset } = range
@@ -665,7 +665,7 @@ Changes.deleteForwardAtRange = (change, range, n = 1, options = {}) => {
 
 Changes.insertBlockAtRange = (change, range, block, options = {}) => {
   block = Block.create(block)
-  const { normalize = true } = options
+  const normalize = change.getFlag('normalize', options)
 
   if (range.isExpanded) {
     change.deleteAtRange(range)
@@ -717,7 +717,7 @@ Changes.insertBlockAtRange = (change, range, block, options = {}) => {
  */
 
 Changes.insertFragmentAtRange = (change, range, fragment, options = {}) => {
-  const { normalize = true } = options
+  const normalize = change.getFlag('normalize', options)
 
   // If the range is expanded, delete it first.
   if (range.isExpanded) {
@@ -830,7 +830,7 @@ Changes.insertFragmentAtRange = (change, range, fragment, options = {}) => {
  */
 
 Changes.insertInlineAtRange = (change, range, inline, options = {}) => {
-  const { normalize = true } = options
+  const normalize = change.getFlag('normalize', options)
   inline = Inline.create(inline)
 
   if (range.isExpanded) {
@@ -908,7 +908,7 @@ Changes.insertTextAtRange = (change, range, text, marks, options = {}) => {
 Changes.removeMarkAtRange = (change, range, mark, options = {}) => {
   if (range.isCollapsed) return
 
-  const { normalize = true } = options
+  const normalize = change.getFlag('normalize', options)
   const { value } = change
   const { document } = value
   const texts = document.getTextsAtRange(range)
@@ -938,7 +938,7 @@ Changes.removeMarkAtRange = (change, range, mark, options = {}) => {
  */
 
 Changes.setBlockAtRange = (change, range, properties, options = {}) => {
-  const { normalize = true } = options
+  const normalize = change.getFlag('normalize', options)
   const { value } = change
   const { document } = value
   const blocks = document.getBlocksAtRange(range)
@@ -959,7 +959,7 @@ Changes.setBlockAtRange = (change, range, properties, options = {}) => {
  */
 
 Changes.setInlineAtRange = (change, range, properties, options = {}) => {
-  const { normalize = true } = options
+  const normalize = change.getFlag('normalize', options)
   const { value } = change
   const { document } = value
   const inlines = document.getInlinesAtRange(range)
@@ -980,7 +980,7 @@ Changes.setInlineAtRange = (change, range, properties, options = {}) => {
  */
 
 Changes.splitBlockAtRange = (change, range, height = 1, options = {}) => {
-  const { normalize = true } = options
+  const normalize = change.getFlag('normalize', options)
 
   if (range.isExpanded) {
     change.deleteAtRange(range, { normalize })
@@ -1014,7 +1014,7 @@ Changes.splitBlockAtRange = (change, range, height = 1, options = {}) => {
  */
 
 Changes.splitInlineAtRange = (change, range, height = Infinity, options = {}) => {
-  const { normalize = true } = options
+  const normalize = change.getFlag('normalize', options)
 
   if (range.isExpanded) {
     change.deleteAtRange(range, { normalize })
@@ -1053,7 +1053,7 @@ Changes.toggleMarkAtRange = (change, range, mark, options = {}) => {
 
   mark = Mark.create(mark)
 
-  const { normalize = true } = options
+  const normalize = change.getFlag('normalize', options)
   const { value } = change
   const { document } = value
   const marks = document.getActiveMarksAtRange(range)
@@ -1079,7 +1079,7 @@ Changes.toggleMarkAtRange = (change, range, mark, options = {}) => {
 Changes.unwrapBlockAtRange = (change, range, properties, options = {}) => {
   properties = Node.createProperties(properties)
 
-  const { normalize = true } = options
+  const normalize = change.getFlag('normalize', options)
   const { value } = change
   let { document } = value
   const blocks = document.getBlocksAtRange(range)
@@ -1171,7 +1171,7 @@ Changes.unwrapBlockAtRange = (change, range, properties, options = {}) => {
 Changes.unwrapInlineAtRange = (change, range, properties, options = {}) => {
   properties = Node.createProperties(properties)
 
-  const { normalize = true } = options
+  const normalize = change.getFlag('normalize', options)
   const { value } = change
   const { document } = value
   const texts = document.getTextsAtRange(range)
@@ -1218,7 +1218,7 @@ Changes.wrapBlockAtRange = (change, range, block, options = {}) => {
   block = Block.create(block)
   block = block.set('nodes', block.nodes.clear())
 
-  const { normalize = true } = options
+  const normalize = change.getFlag('normalize', options)
   const { value } = change
   const { document } = value
 
@@ -1288,7 +1288,7 @@ Changes.wrapBlockAtRange = (change, range, block, options = {}) => {
 Changes.wrapInlineAtRange = (change, range, inline, options = {}) => {
   const { value } = change
   let { document } = value
-  const { normalize = true } = options
+  const normalize = change.getFlag('normalize', options)
   const { startKey, startOffset, endKey, endOffset } = range
 
   if (range.isCollapsed) {
@@ -1397,7 +1397,7 @@ Changes.wrapInlineAtRange = (change, range, inline, options = {}) => {
  */
 
 Changes.wrapTextAtRange = (change, range, prefix, suffix = prefix, options = {}) => {
-  const { normalize = true } = options
+  const normalize = change.getFlag('normalize', options)
   const { startKey, endKey } = range
   const start = range.collapseToStart()
   let end = range.collapseToEnd()

packages/slate/src/changes/by-key.js

@@ -26,7 +26,7 @@ const Changes = {}
 
 Changes.addMarkByKey = (change, key, offset, length, mark, options = {}) => {
   mark = Mark.create(mark)
-  const { normalize = true } = options
+  const normalize = change.getFlag('normalize', options)
   const { value } = change
   const { document } = value
   const path = document.getPath(key)
@@ -84,7 +84,7 @@ Changes.addMarkByKey = (change, key, offset, length, mark, options = {}) => {
  */
 
 Changes.insertFragmentByKey = (change, key, index, fragment, options = {}) => {
-  const { normalize = true } = options
+  const normalize = change.getFlag('normalize', options)
 
   fragment.nodes.forEach((node, i) => {
     change.insertNodeByKey(key, index + i, node)
@@ -107,7 +107,7 @@ Changes.insertFragmentByKey = (change, key, index, fragment, options = {}) => {
  */
 
 Changes.insertNodeByKey = (change, key, index, node, options = {}) => {
-  const { normalize = true } = options
+  const normalize = change.getFlag('normalize', options)
   const { value } = change
   const { document } = value
   const path = document.getPath(key)
@@ -137,7 +137,8 @@ Changes.insertNodeByKey = (change, key, index, node, options = {}) => {
  */
 
 Changes.insertTextByKey = (change, key, offset, text, marks, options = {}) => {
-  const { normalize = true } = options
+  const normalize = change.getFlag('normalize', options)
+
   const { value } = change
   const { document } = value
   const path = document.getPath(key)
@@ -169,7 +170,7 @@ Changes.insertTextByKey = (change, key, offset, text, marks, options = {}) => {
  */
 
 Changes.mergeNodeByKey = (change, key, options = {}) => {
-  const { normalize = true } = options
+  const normalize = change.getFlag('normalize', options)
   const { value } = change
   const { document } = value
   const path = document.getPath(key)
@@ -208,7 +209,7 @@ Changes.mergeNodeByKey = (change, key, options = {}) => {
  */
 
 Changes.moveNodeByKey = (change, key, newKey, newIndex, options = {}) => {
-  const { normalize = true } = options
+  const normalize = change.getFlag('normalize', options)
   const { value } = change
   const { document } = value
   const path = document.getPath(key)
@@ -241,7 +242,7 @@ Changes.moveNodeByKey = (change, key, newKey, newIndex, options = {}) => {
 
 Changes.removeMarkByKey = (change, key, offset, length, mark, options = {}) => {
   mark = Mark.create(mark)
-  const { normalize = true } = options
+  const normalize = change.getFlag('normalize', options)
   const { value } = change
   const { document } = value
   const path = document.getPath(key)
@@ -319,7 +320,7 @@ Changes.removeAllMarksByKey = (change, key, options = {}) => {
  */
 
 Changes.removeNodeByKey = (change, key, options = {}) => {
-  const { normalize = true } = options
+  const normalize = change.getFlag('normalize', options)
   const { value } = change
   const { document } = value
   const path = document.getPath(key)
@@ -350,7 +351,7 @@ Changes.removeNodeByKey = (change, key, options = {}) => {
  */
 
 Changes.removeTextByKey = (change, key, offset, length, options = {}) => {
-  const { normalize = true } = options
+  const normalize = change.getFlag('normalize', options)
   const { value } = change
   const { document } = value
   const path = document.getPath(key)
@@ -408,7 +409,7 @@ Changes.removeTextByKey = (change, key, offset, length, options = {}) => {
 
 Changes.replaceNodeByKey = (change, key, newNode, options = {}) => {
   newNode = Node.create(newNode)
-  const { normalize = true } = options
+  const normalize = change.getFlag('normalize', options)
   const { value } = change
   const { document } = value
   const node = document.getNode(key)
@@ -436,7 +437,7 @@ Changes.replaceNodeByKey = (change, key, newNode, options = {}) => {
 Changes.setMarkByKey = (change, key, offset, length, mark, properties, options = {}) => {
   mark = Mark.create(mark)
   properties = Mark.createProperties(properties)
-  const { normalize = true } = options
+  const normalize = change.getFlag('normalize', options)
   const { value } = change
   const { document } = value
   const path = document.getPath(key)
@@ -469,7 +470,7 @@ Changes.setMarkByKey = (change, key, offset, length, mark, properties, options =
 
 Changes.setNodeByKey = (change, key, properties, options = {}) => {
   properties = Node.createProperties(properties)
-  const { normalize = true } = options
+  const normalize = change.getFlag('normalize', options)
   const { value } = change
   const { document } = value
   const path = document.getPath(key)
@@ -534,7 +535,7 @@ Changes.splitDescendantsByKey = (change, key, textKey, textOffset, options = {})
     return
   }
 
-  const { normalize = true } = options
+  const normalize = change.getFlag('normalize', options)
   const { value } = change
   const { document } = value
 
@@ -611,7 +612,7 @@ Changes.unwrapBlockByKey = (change, key, properties, options) => {
  */
 
 Changes.unwrapNodeByKey = (change, key, options = {}) => {
-  const { normalize = true } = options
+  const normalize = change.getFlag('normalize', options)
   const { value } = change
   const { document } = value
   const parent = document.getParent(key)

packages/slate/src/models/change.js

@@ -48,7 +48,10 @@ class Change {
     const { value } = attrs
     this.value = value
     this.operations = new List()
-    this.flags = pick(attrs, ['merge', 'save'])
+    this.flags = {
+      normalize: true,
+      ...pick(attrs, ['merge', 'save', 'normalize'])
+    }
   }
 
   /**
@@ -140,6 +143,27 @@ class Change {
     return this
   }
 
+  /**
+   * Applies a series of change mutations and defers normalization until the end.
+   *
+   * @param {Function} customChange - function that accepts a change object and executes change operations
+   * @return {Change}
+   */
+
+  withoutNormalization(customChange) {
+    const original = this.flags.normalize
+    this.setOperationFlag('normalize', false)
+    try {
+      customChange(this)
+      // if the change function worked then run normalization
+      this.normalizeDocument()
+    } finally {
+      // restore the flag to whatever it was
+      this.setOperationFlag('normalize', original)
+    }
+    return this
+  }
+
   /**
    * Set an operation flag by `key` to `value`.
    *
@@ -153,6 +177,21 @@ class Change {
     return this
   }
 
+  /**
+   * Get the `value` of the specified flag by its `key`. Optionally accepts an `options`
+   * object with override flags.
+   *
+   * @param {String} key
+   * @param {Object} options
+   * @return {Change}
+   */
+
+  getFlag(key, options = {}) {
+    return options[key] !== undefined ?
+      options[key] :
+      this.flags[key]
+  }
+
   /**
    * Unset an operation flag by `key`.
    *

packages/slate/test/index.js

@@ -15,6 +15,7 @@ describe('slate', () => {
   require('./changes')
   require('./history')
   require('./operations')
+  require('./models')
 })
 
 /**

packages/slate/test/models/change/without-normalization-normalize-flag-false.js

@@ -0,0 +1,48 @@
+/** @jsx h */
+
+import h from '../../helpers/h'
+
+
+export const flags = { normalize: false }
+
+export const schema = {
+  blocks: {
+    paragraph: {},
+    item: {
+      parent: { types: ['list'] },
+      nodes: [
+        { objects: ['text'] }
+      ]
+    },
+    list: {},
+  }
+}
+
+export const customChange = (change) => {
+  // this change function and schema are designed such that if
+  // validation takes place before both wrapBlock calls complete
+  // the node gets deleted by the default schema
+  // and causes a test failure
+  let target = change.value.document.nodes.get(0)
+  change.wrapBlockByKey(target.key, 'item')
+  target = change.value.document.nodes.get(0)
+  change.wrapBlockByKey(target.key, 'list')
+}
+
+export const input = (
+  <value>
+    <document>
+      <paragraph />
+    </document>
+  </value>
+)
+
+export const output = (
+  <value>
+    <document>
+      <list>
+        <item />
+      </list>
+    </document>
+  </value>
+)

packages/slate/test/models/change/without-normalization-normalize-flag-true.js

@@ -0,0 +1,48 @@
+/** @jsx h */
+
+import h from '../../helpers/h'
+
+
+export const flags = { normalize: true }
+
+export const schema = {
+  blocks: {
+    paragraph: {},
+    item: {
+      parent: { types: ['list'] },
+      nodes: [
+        { objects: ['text'] }
+      ]
+    },
+    list: {},
+  }
+}
+
+export const customChange = (change) => {
+  // this change function and schema are designed such that if
+  // validation takes place before both wrapBlock calls complete
+  // the node gets deleted by the default schema
+  // and causes a test failure
+  let target = change.value.document.nodes.get(0)
+  change.wrapBlockByKey(target.key, 'item')
+  target = change.value.document.nodes.get(0)
+  change.wrapBlockByKey(target.key, 'list')
+}
+
+export const input = (
+  <value>
+    <document>
+      <paragraph />
+    </document>
+  </value>
+)
+
+export const output = (
+  <value>
+    <document>
+      <list>
+        <item />
+      </list>
+    </document>
+  </value>
+)

packages/slate/test/models/change/without-normalization-option-override.js

@@ -0,0 +1,40 @@
+/** @jsx h */
+
+import h from '../../helpers/h'
+
+
+export const flags = { }
+
+export const schema = {
+  blocks: {
+    paragraph: {},
+    item: {
+      parent: { types: ['list'] },
+      nodes: [
+        { objects: ['text'] }
+      ]
+    },
+    list: {},
+  }
+}
+
+export const customChange = (change) => {
+  // see if we can break the expected validation sequence by toggling
+  // the normalization option
+  const target = change.value.document.nodes.get(0)
+  change.wrapBlockByKey(target.key, 'item', { normalize: true })
+}
+
+export const input = (
+  <value>
+    <document>
+      <paragraph />
+    </document>
+  </value>
+)
+
+export const output = (
+  <value>
+    <document />
+  </value>
+)

packages/slate/test/models/change/without-normalization.js

@@ -0,0 +1,47 @@
+/** @jsx h */
+
+import h from '../../helpers/h'
+
+export const flags = { }
+
+export const schema = {
+  blocks: {
+    paragraph: {},
+    item: {
+      parent: { types: ['list'] },
+      nodes: [
+        { objects: ['text'] }
+      ]
+    },
+    list: {},
+  }
+}
+
+export const customChange = (change) => {
+  // this change function and schema are designed such that if
+  // validation takes place before both wrapBlock calls complete
+  // the node gets deleted by the default schema
+  // and causes a test failure
+  let target = change.value.document.nodes.get(0)
+  change.wrapBlockByKey(target.key, 'item')
+  target = change.value.document.nodes.get(0)
+  change.wrapBlockByKey(target.key, 'list')
+}
+
+export const input = (
+  <value>
+    <document>
+      <paragraph />
+    </document>
+  </value>
+)
+
+export const output = (
+  <value>
+    <document>
+      <list>
+        <item />
+      </list>
+    </document>
+  </value>
+)

packages/slate/test/models/index.js

@@ -0,0 +1,34 @@
+
+import assert from 'assert'
+import fs from 'fs'
+import { Schema } from '../..'
+import { basename, extname, resolve } from 'path'
+
+/**
+ * Tests.
+ */
+
+describe('models', () => {
+  describe('change', () => {
+    describe('withoutNormalization', () => {
+      const testsDir = resolve(__dirname, 'change')
+      const tests = fs.readdirSync(testsDir).filter(t => t[0] != '.').map(t => basename(t, extname(t)))
+
+      for (const test of tests) {
+        it(test, async () => {
+          const module = require(resolve(testsDir, test))
+          const { input, output, schema, flags, customChange } = module
+          const s = Schema.create(schema)
+          const expected = output.toJSON()
+          const actual = input
+            .change(flags)
+            .setValue({ schema: s })
+            .withoutNormalization(customChange)
+            .value.toJSON()
+
+          assert.deepEqual(actual, expected)
+        })
+      }
+    })
+  })
+})