diff --git a/docs/guides/schemas.md b/docs/guides/schemas.md index e19e3f5af..d532046e0 100644 --- a/docs/guides/schemas.md +++ b/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) + }) + }); + } +} +``` \ No newline at end of file diff --git a/docs/reference/slate/change.md b/docs/reference/slate/change.md index 0c9d3f824..d847423bc 100644 --- a/docs/reference/slate/change.md +++ b/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 diff --git a/packages/slate/src/changes/at-range.js b/packages/slate/src/changes/at-range.js index f33acbf6f..3f6e18105 100644 --- a/packages/slate/src/changes/at-range.js +++ b/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() diff --git a/packages/slate/src/changes/by-key.js b/packages/slate/src/changes/by-key.js index 791f52259..49f51e2f6 100644 --- a/packages/slate/src/changes/by-key.js +++ b/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) diff --git a/packages/slate/src/models/change.js b/packages/slate/src/models/change.js index 2596c43ec..00ae550d5 100644 --- a/packages/slate/src/models/change.js +++ b/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`. * diff --git a/packages/slate/test/index.js b/packages/slate/test/index.js index f90e52690..ebae54271 100644 --- a/packages/slate/test/index.js +++ b/packages/slate/test/index.js @@ -15,6 +15,7 @@ describe('slate', () => { require('./changes') require('./history') require('./operations') + require('./models') }) /** diff --git a/packages/slate/test/models/change/without-normalization-normalize-flag-false.js b/packages/slate/test/models/change/without-normalization-normalize-flag-false.js new file mode 100644 index 000000000..0967041fd --- /dev/null +++ b/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 = ( + + + + + +) + +export const output = ( + + + + + + + +) diff --git a/packages/slate/test/models/change/without-normalization-normalize-flag-true.js b/packages/slate/test/models/change/without-normalization-normalize-flag-true.js new file mode 100644 index 000000000..671bbea62 --- /dev/null +++ b/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 = ( + + + + + +) + +export const output = ( + + + + + + + +) diff --git a/packages/slate/test/models/change/without-normalization-option-override.js b/packages/slate/test/models/change/without-normalization-option-override.js new file mode 100644 index 000000000..fd56859be --- /dev/null +++ b/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 = ( + + + + + +) + +export const output = ( + + + +) diff --git a/packages/slate/test/models/change/without-normalization.js b/packages/slate/test/models/change/without-normalization.js new file mode 100644 index 000000000..d55edc4ee --- /dev/null +++ b/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 = ( + + + + + +) + +export const output = ( + + + + + + + +) diff --git a/packages/slate/test/models/index.js b/packages/slate/test/models/index.js new file mode 100644 index 000000000..d8a722ad0 --- /dev/null +++ b/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) + }) + } + }) + }) +})