Make all ops invertible and remove value from ops (#2225)

packages/slate/src/commands/by-path.js

@@ -1,3 +1,4 @@
+import pick from 'lodash/pick'
 import Block from '../models/block'
 import Inline from '../models/inline'
 import Mark from '../models/mark'
@@ -52,7 +53,6 @@ Commands.addMarkByPath = (editor, path, offset, length, mark) => {
 
     operations.push({
       type: 'add_mark',
-      value,
       path,
       offset: start,
       length: end - start,
@@ -88,11 +88,8 @@ Commands.insertFragmentByPath = (editor, path, index, fragment) => {
  */
 
 Commands.insertNodeByPath = (editor, path, index, node) => {
-  const { value } = editor
-
   editor.applyOperation({
     type: 'insert_node',
-    value,
     path: path.concat(index),
     node,
   })
@@ -137,7 +134,6 @@ Commands.insertTextByPath = (editor, path, offset, text, marks) => {
 
   editor.applyOperation({
     type: 'insert_text',
-    value,
     path,
     offset,
     text,
@@ -169,7 +165,6 @@ Commands.mergeNodeByPath = (editor, path) => {
 
   editor.applyOperation({
     type: 'merge_node',
-    value,
     path,
     position,
     // for undos to succeed we only need the type and data because
@@ -192,8 +187,6 @@ Commands.mergeNodeByPath = (editor, path) => {
  */
 
 Commands.moveNodeByPath = (editor, path, newParentPath, newIndex) => {
-  const { value } = editor
-
   // If the operation path and newParentPath are the same,
   // this should be considered a NOOP
   if (PathUtils.isEqual(path, newParentPath)) {
@@ -208,7 +201,6 @@ Commands.moveNodeByPath = (editor, path, newParentPath, newIndex) => {
 
   editor.applyOperation({
     type: 'move_node',
-    value,
     path,
     newPath,
   })
@@ -254,7 +246,6 @@ Commands.removeMarkByPath = (editor, path, offset, length, mark) => {
 
     operations.push({
       type: 'remove_mark',
-      value,
       path,
       offset: start,
       length: end - start,
@@ -299,7 +290,6 @@ Commands.removeNodeByPath = (editor, path) => {
 
   editor.applyOperation({
     type: 'remove_node',
-    value,
     path,
     node,
   })
@@ -370,7 +360,6 @@ Commands.removeTextByPath = (editor, path, offset, length) => {
 
     removals.push({
       type: 'remove_text',
-      value,
       path,
       offset: start,
       text: string,
@@ -447,28 +436,35 @@ Commands.replaceTextByPath = (editor, path, offset, length, text, marks) => {
 }
 
 /**
- * Set `properties` on mark on text at `offset` and `length` in node by `path`.
+ * Set `newProperties` on mark on text at `offset` and `length` in node by `path`.
  *
  * @param {Editor} editor
  * @param {Array} path
  * @param {Number} offset
  * @param {Number} length
- * @param {Mark} mark
+ * @param {Object|Mark} properties
+ * @param {Object} newProperties
  */
 
-Commands.setMarkByPath = (editor, path, offset, length, mark, properties) => {
-  mark = Mark.create(mark)
-  properties = Mark.createProperties(properties)
-  const { value } = editor
+Commands.setMarkByPath = (
+  editor,
+  path,
+  offset,
+  length,
+  properties,
+  newProperties
+) => {
+  // we call Mark.create() here because we need the complete previous mark instance
+  properties = Mark.create(properties)
+  newProperties = Mark.createProperties(newProperties)
 
   editor.applyOperation({
     type: 'set_mark',
-    value,
     path,
     offset,
     length,
-    mark,
     properties,
+    newProperties,
   })
 }
 
@@ -477,21 +473,21 @@ Commands.setMarkByPath = (editor, path, offset, length, mark, properties) => {
  *
  * @param {Editor} editor
  * @param {Array} path
- * @param {Object|String} properties
+ * @param {Object|String} newProperties
  */
 
-Commands.setNodeByPath = (editor, path, properties) => {
-  properties = Node.createProperties(properties)
+Commands.setNodeByPath = (editor, path, newProperties) => {
   const { value } = editor
   const { document } = value
   const node = document.assertNode(path)
+  newProperties = Node.createProperties(newProperties)
+  const prevProperties = pick(node, Object.keys(newProperties))
 
   editor.applyOperation({
     type: 'set_node',
-    value,
     path,
-    node,
-    properties,
+    properties: prevProperties,
+    newProperties,
   })
 }
 
@@ -529,7 +525,6 @@ Commands.splitNodeByPath = (editor, path, position, options = {}) => {
 
   editor.applyOperation({
     type: 'split_node',
-    value,
     path,
     position,
     target,

packages/slate/src/commands/on-selection.js

@@ -592,7 +592,7 @@ Commands.select = (editor, properties, options = {}) => {
   const { snapshot = false } = options
   const { value } = editor
   const { document, selection } = value
-  const props = {}
+  const newProperties = {}
   let next = selection.setProperties(properties)
   next = document.resolveSelection(next)
 
@@ -604,27 +604,34 @@ Commands.select = (editor, properties, options = {}) => {
   // are being changed, for the inverse operation.
   for (const k in properties) {
     if (snapshot === true || !is(properties[k], selection[k])) {
-      props[k] = properties[k]
+      newProperties[k] = properties[k]
     }
   }
 
   // If the selection moves, clear any marks, unless the new selection
-  // properties editor the marks in some way.
-  if (selection.marks && !props.marks && (props.anchor || props.focus)) {
-    props.marks = null
+  // properties change the marks in some way.
+  if (
+    selection.marks &&
+    !newProperties.marks &&
+    (newProperties.anchor || newProperties.focus)
+  ) {
+    newProperties.marks = null
   }
 
   // If there are no new properties to set, abort to avoid extra operations.
-  if (Object.keys(props).length === 0) {
+  if (Object.keys(newProperties).length === 0) {
     return
   }
 
+  // TODO: for some reason toJSON() is required here (it breaks selections between blocks)? - 2018-10-10
+  const prevProperties = pick(selection.toJSON(), Object.keys(newProperties))
+
   editor.applyOperation(
     {
       type: 'set_selection',
       value,
-      properties: props,
-      selection: selection.toJSON(),
+      properties: prevProperties,
+      newProperties,
     },
     snapshot ? { skip: false, merge: false } : {}
   )

packages/slate/src/commands/on-value.js

@@ -1,3 +1,4 @@
+import pick from 'lodash/pick'
 import Value from '../models/value'
 
 /**
@@ -16,13 +17,14 @@ const Commands = {}
  */
 
 Commands.setData = (editor, data = {}) => {
-  const properties = Value.createProperties({ data })
   const { value } = editor
+  const newProperties = Value.createProperties({ data })
+  const prevProperties = pick(value, Object.keys(newProperties))
 
   editor.applyOperation({
     type: 'set_value',
-    properties,
-    value,
+    properties: prevProperties,
+    newProperties,
   })
 }
 
@@ -34,13 +36,14 @@ Commands.setData = (editor, data = {}) => {
  */
 
 Commands.setDecorations = (editor, decorations = []) => {
-  const properties = Value.createProperties({ decorations })
   const { value } = editor
+  const newProperties = Value.createProperties({ decorations })
+  const prevProperties = pick(value, Object.keys(newProperties))
 
   editor.applyOperation({
     type: 'set_value',
-    properties,
-    value,
+    properties: prevProperties,
+    newProperties,
   })
 }
 

packages/slate/src/interfaces/element.js

@@ -2240,9 +2240,9 @@ class ElementInterface {
    * @return {Node}
    */
 
-  setMark(path, offset, length, mark, properties) {
+  setMark(path, offset, length, properties, newProperties) {
     let node = this.assertNode(path)
-    node = node.updateMark(offset, length, mark, properties)
+    node = node.updateMark(offset, length, properties, newProperties)
     const ret = this.replaceNode(path, node)
     return ret
   }

packages/slate/src/models/operation.js

@@ -16,19 +16,19 @@ import invert from '../operations/invert'
  */
 
 const OPERATION_ATTRIBUTES = {
-  add_mark: ['value', 'path', 'offset', 'length', 'mark', 'data'],
-  insert_node: ['value', 'path', 'node', 'data'],
-  insert_text: ['value', 'path', 'offset', 'text', 'marks', 'data'],
-  merge_node: ['value', 'path', 'position', 'properties', 'target', 'data'],
-  move_node: ['value', 'path', 'newPath', 'data'],
-  remove_mark: ['value', 'path', 'offset', 'length', 'mark', 'data'],
-  remove_node: ['value', 'path', 'node', 'data'],
-  remove_text: ['value', 'path', 'offset', 'text', 'marks', 'data'],
-  set_mark: ['value', 'path', 'offset', 'length', 'mark', 'properties', 'data'],
-  set_node: ['value', 'path', 'node', 'properties', 'data'],
-  set_selection: ['value', 'selection', 'properties', 'data'],
-  set_value: ['value', 'properties', 'data'],
-  split_node: ['value', 'path', 'position', 'properties', 'target', 'data'],
+  add_mark: ['path', 'offset', 'length', 'mark', 'data'],
+  insert_node: ['path', 'node', 'data'],
+  insert_text: ['path', 'offset', 'text', 'marks', 'data'],
+  merge_node: ['path', 'position', 'properties', 'target', 'data'],
+  move_node: ['path', 'newPath', 'data'],
+  remove_mark: ['path', 'offset', 'length', 'mark', 'data'],
+  remove_node: ['path', 'node', 'data'],
+  remove_text: ['path', 'offset', 'text', 'marks', 'data'],
+  set_mark: ['path', 'offset', 'length', 'properties', 'newProperties', 'data'],
+  set_node: ['path', 'properties', 'newProperties', 'data'],
+  set_selection: ['properties', 'newProperties', 'data'],
+  set_value: ['properties', 'newProperties', 'data'],
+  split_node: ['path', 'position', 'properties', 'target', 'data'],
 }
 
 /**
@@ -47,11 +47,10 @@ const DEFAULTS = {
   path: undefined,
   position: undefined,
   properties: undefined,
-  selection: undefined,
+  newProperties: undefined,
   target: undefined,
   text: undefined,
   type: undefined,
-  value: undefined,
   data: undefined,
 }
 
@@ -132,13 +131,6 @@ class Operation extends Record(DEFAULTS) {
       }
 
       if (v === undefined) {
-        // Skip keys for objects that should not be serialized, and are only used
-        // for providing the local-only invert behavior for the history stack.
-        if (key === 'document') continue
-        if (key === 'selection') continue
-        if (key === 'value') continue
-        if (key === 'node' && type !== 'insert_node') continue
-
         throw new Error(
           `\`Operation.fromJSON\` was passed a "${type}" operation without the required "${key}" attribute.`
         )
@@ -160,31 +152,35 @@ class Operation extends Record(DEFAULTS) {
         v = Node.create(v)
       }
 
-      if (key === 'selection') {
-        v = Selection.create(v)
-      }
-
-      if (key === 'value') {
-        v = Value.create(v)
-      }
-
       if (key === 'properties' && type === 'merge_node') {
         v = Node.createProperties(v)
       }
 
-      if (key === 'properties' && type === 'set_mark') {
+      if (
+        (key === 'properties' || key === 'newProperties') &&
+        type === 'set_mark'
+      ) {
         v = Mark.createProperties(v)
       }
 
-      if (key === 'properties' && type === 'set_node') {
+      if (
+        (key === 'properties' || key === 'newProperties') &&
+        type === 'set_node'
+      ) {
         v = Node.createProperties(v)
       }
 
-      if (key === 'properties' && type === 'set_selection') {
+      if (
+        (key === 'properties' || key === 'newProperties') &&
+        type === 'set_selection'
+      ) {
         v = Selection.createProperties(v)
       }
 
-      if (key === 'properties' && type === 'set_value') {
+      if (
+        (key === 'properties' || key === 'newProperties') &&
+        type === 'set_value'
+      ) {
         v = Value.createProperties(v)
       }
 
@@ -252,13 +248,6 @@ class Operation extends Record(DEFAULTS) {
     for (const key of ATTRIBUTES) {
       let value = this[key]
 
-      // Skip keys for objects that should not be serialized, and are only used
-      // for providing the local-only invert behavior for the history stack.
-      if (key === 'document') continue
-      if (key === 'selection') continue
-      if (key === 'value') continue
-      if (key === 'node' && type !== 'insert_node') continue
-
       if (
         key === 'mark' ||
         key === 'marks' ||
@@ -276,21 +265,30 @@ class Operation extends Record(DEFAULTS) {
         value = v
       }
 
-      if (key === 'properties' && type === 'set_mark') {
+      if (
+        (key === 'properties' || key === 'newProperties') &&
+        type === 'set_mark'
+      ) {
         const v = {}
         if ('data' in value) v.data = value.data.toJS()
         if ('type' in value) v.type = value.type
         value = v
       }
 
-      if (key === 'properties' && type === 'set_node') {
+      if (
+        (key === 'properties' || key === 'newProperties') &&
+        type === 'set_node'
+      ) {
         const v = {}
         if ('data' in value) v.data = value.data.toJS()
         if ('type' in value) v.type = value.type
         value = v
       }
 
-      if (key === 'properties' && type === 'set_selection') {
+      if (
+        (key === 'properties' || key === 'newProperties') &&
+        type === 'set_selection'
+      ) {
         const v = {}
         if ('anchor' in value) v.anchor = value.anchor.toJSON()
         if ('focus' in value) v.focus = value.focus.toJSON()
@@ -299,7 +297,10 @@ class Operation extends Record(DEFAULTS) {
         value = v
       }
 
-      if (key === 'properties' && type === 'set_value') {
+      if (
+        (key === 'properties' || key === 'newProperties') &&
+        type === 'set_value'
+      ) {
         const v = {}
         if ('data' in value) v.data = value.data.toJS()
         if ('decorations' in value) v.decorations = value.decorations.toJS()

packages/slate/src/models/text.js

@@ -2,6 +2,7 @@ import isPlainObject from 'is-plain-object'
 import warning from 'tiny-warning'
 import { List, OrderedSet, Record, Set } from 'immutable'
 
+import Mark from './mark'
 import Leaf from './leaf'
 import KeyUtils from '../utils/key-utils'
 import memoize from '../utils/memoize'
@@ -572,13 +573,14 @@ class Text extends Record(DEFAULTS) {
    *
    * @param {Number} index
    * @param {Number} length
-   * @param {Mark} mark
    * @param {Object} properties
+   * @param {Object} newProperties
    * @return {Text}
    */
 
-  updateMark(index, length, mark, properties) {
-    const newMark = mark.merge(properties)
+  updateMark(index, length, properties, newProperties) {
+    const mark = Mark.create(properties)
+    const newMark = mark.merge(newProperties)
 
     if (this.text === '' && length === 0 && index === 0) {
       const { leaves } = this

packages/slate/src/operations/apply.js

@@ -79,26 +79,32 @@ function applyOperation(value, op) {
     }
 
     case 'set_mark': {
-      const { path, offset, length, mark, properties } = op
-      const next = value.setMark(path, offset, length, mark, properties)
+      const { path, offset, length, properties, newProperties } = op
+      const next = value.setMark(
+        path,
+        offset,
+        length,
+        properties,
+        newProperties
+      )
       return next
     }
 
     case 'set_node': {
-      const { path, properties } = op
-      const next = value.setNode(path, properties)
+      const { path, newProperties } = op
+      const next = value.setNode(path, newProperties)
       return next
     }
 
     case 'set_selection': {
-      const { properties } = op
-      const next = value.setSelection(properties)
+      const { newProperties } = op
+      const next = value.setSelection(newProperties)
       return next
     }
 
     case 'set_value': {
-      const { properties } = op
-      const next = value.setProperties(properties)
+      const { newProperties } = op
+      const next = value.setProperties(newProperties)
       return next
     }
 

packages/slate/src/operations/invert.js

@@ -1,5 +1,4 @@
 import Debug from 'debug'
-import pick from 'lodash/pick'
 
 import Operation from '../models/operation'
 import PathUtils from '../utils/path-utils'
@@ -74,13 +73,14 @@ function invertOperation(op) {
       return inverse
     }
 
-    case 'set_node': {
-      const { properties, node } = op
-      const inverseNode = node.merge(properties)
-      const inverseProperties = pick(node, Object.keys(properties))
+    case 'set_node':
+    case 'set_value':
+    case 'set_selection':
+    case 'set_mark': {
+      const { properties, newProperties } = op
       const inverse = op
-        .set('node', inverseNode)
-        .set('properties', inverseProperties)
+        .set('properties', newProperties)
+        .set('newProperties', properties)
       return inverse
     }
 
@@ -104,36 +104,6 @@ function invertOperation(op) {
       return inverse
     }
 
-    case 'set_mark': {
-      const { properties, mark } = op
-      const inverseMark = mark.merge(properties)
-      const inverseProperties = pick(mark, Object.keys(properties))
-      const inverse = op
-        .set('mark', inverseMark)
-        .set('properties', inverseProperties)
-      return inverse
-    }
-
-    case 'set_selection': {
-      const { properties, selection } = op
-      const inverseSelection = selection.merge(properties)
-      const inverseProps = pick(selection, Object.keys(properties))
-      const inverse = op
-        .set('selection', inverseSelection)
-        .set('properties', inverseProps)
-      return inverse
-    }
-
-    case 'set_value': {
-      const { properties, value } = op
-      const inverseValue = value.merge(properties)
-      const inverseProperties = pick(value, Object.keys(properties))
-      const inverse = op
-        .set('value', inverseValue)
-        .set('properties', inverseProperties)
-      return inverse
-    }
-
     default: {
       throw new Error(`Unknown operation type: "${type}".`)
     }

packages/slate/test/operations/apply/remove-node/decoration-across-blocks.js

@@ -6,6 +6,11 @@ export default [
   {
     type: 'remove_node',
     path: [0],
+    node: (
+      <paragraph>
+        o<highlight key="a" />ne
+      </paragraph>
+    ),
   },
 ]