Slate: nested validation stacks break because they've already been normalized

[ianstormtaylor]

Do you want to request a feature or report a bug?

Bug.

What's the current behavior?

If you write a custom normalize function for a schema, and it happens to do something like change.moveNodeByKey, there's a chance that calling that change method will initialize its own validation stack, fixing problems that the parent validation stack assumes are still there.

For example, consider that you have two child text nodes adjacent to each other, which is invalid in Slate. If in your normalize function you make any change to the parent of those text nodes, that change will end up normalizing internally, and the text nodes will be combined. However, the parent validation stack still thinks that an invalid rule that needs to be fixed, so it tries to normalize them again, and fails.

What's the expected behavior?

Not sure.

The ideal would be to prevent the double-checking from happening. I haven't dug into this yet, so I'm not sure if it's impossible to do or not.

Another option would be to automatically pass options.normalize: false to all change methods called while performing a validations. The "core" validateNode handlers already do this automatically. But this feels like it might be too magic-y?

Have others run into this? Thoughts?

[Soreine]

I think I just ran into this https://github.com/ianstormtaylor/slate/issues/1378
In my example, adding options.normalize: false to the change done in the custom normalization does not fix the issue.

However, the parent validation stack still thinks that an invalid rule that needs to be fixed, so it tries to normalize them again, and fails.

What precisely makes it "thinks that it still needs to be fixed"? Is it because we are not "refinding" the nodes correctly after a normalization happened?
Why did this problem never arise in the previous implementation?

Should we simply restart validation from the document after every normalization happening ? (heavy...)

[alanchrt]

Pretty sure I'm running into this with wrapBlockByKey. Here's an example trace (pardon webpack indirection) that shows the multiple normalization passes with wrapBlockByKey calling moveNodeByKey internally. In the trace, it's isolated down to a single schema rule and a single validation failure:

Uncaught Error: Could not find a node with key "3".
    at Document.assertNode (http://localhost:3000/static/js/bundle.js:204111:15)
    at Document.getPath (http://localhost:3000/static/js/bundle.js:205363:24)
    at Document.object.(anonymous function) [as getPath] (http://localhost:3000/static/js/bundle.js:210941:30)
    at ./node_modules/slate/lib/changes/by-key.js.Changes.mergeNodeByKey (http://localhost:3000/static/js/bundle.js:199649:23)
    at Change.call (http://localhost:3000/static/js/bundle.js:202102:10)
    at Change.(anonymous function) [as mergeNodeByKey] (http://localhost:3000/static/js/bundle.js:202161:15)
    at http://localhost:3000/static/js/bundle.js:201370:16
    at List.__iterate (http://localhost:3000/static/js/bundle.js:30578:13)
    at List.forEach (http://localhost:3000/static/js/bundle.js:32753:19)
    at http://localhost:3000/static/js/bundle.js:201369:26
    at iterate (http://localhost:3000/static/js/bundle.js:201084:5)
    at normalizeNode (http://localhost:3000/static/js/bundle.js:201105:3)
    at normalizeNodeAndChildren (http://localhost:3000/static/js/bundle.js:201047:5)
    at _loop (http://localhost:3000/static/js/bundle.js:201020:7)
    at normalizeNodeAndChildren (http://localhost:3000/static/js/bundle.js:201042:5)
    at ./node_modules/slate/lib/changes/with-schema.js.Changes.normalizeNodeByKey (http://localhost:3000/static/js/bundle.js:200975:3)
    at Change.call (http://localhost:3000/static/js/bundle.js:202102:10)
    at Change.(anonymous function) [as normalizeNodeByKey] (http://localhost:3000/static/js/bundle.js:202161:15)
    at ./node_modules/slate/lib/changes/by-key.js.Changes.moveNodeByKey (http://localhost:3000/static/js/bundle.js:199700:12)
    at Change.call (http://localhost:3000/static/js/bundle.js:202102:10)
    at Change.(anonymous function) [as moveNodeByKey] (http://localhost:3000/static/js/bundle.js:202161:15)
    at ./node_modules/slate/lib/changes/by-key.js.Changes.wrapBlockByKey (http://localhost:3000/static/js/bundle.js:200181:10)
    at Change.call (http://localhost:3000/static/js/bundle.js:202102:10)
    at Change.(anonymous function) [as wrapBlockByKey] (http://localhost:3000/static/js/bundle.js:202161:15)
    at Object.normalize (http://localhost:3000/static/js/bundle.js:226701:100)
    at http://localhost:3000/static/js/bundle.js:207420:34
    at iterate (http://localhost:3000/static/js/bundle.js:201084:5)
    at normalizeNode (http://localhost:3000/static/js/bundle.js:201105:3)
    at normalizeNodeAndChildren (http://localhost:3000/static/js/bundle.js:201047:5)
    at _loop (http://localhost:3000/static/js/bundle.js:201020:7)
    at normalizeNodeAndChildren (http://localhost:3000/static/js/bundle.js:201042:5)
    at ./node_modules/slate/lib/changes/with-schema.js.Changes.normalizeNodeByKey (http://localhost:3000/static/js/bundle.js:200975:3)
    at Change.call (http://localhost:3000/static/js/bundle.js:202102:10)
    at Change.(anonymous function) [as normalizeNodeByKey] (http://localhost:3000/static/js/bundle.js:202161:15)
    at ./node_modules/slate/lib/changes/by-key.js.Changes.moveNodeByKey (http://localhost:3000/static/js/bundle.js:199700:12)
    at Change.call (http://localhost:3000/static/js/bundle.js:202102:10)
    at Change.(anonymous function) [as moveNodeByKey] (http://localhost:3000/static/js/bundle.js:202161:15)
    at ./node_modules/slate/lib/changes/by-key.js.Changes.wrapBlockByKey (http://localhost:3000/static/js/bundle.js:200181:10)
    at Change.call (http://localhost:3000/static/js/bundle.js:202102:10)
    at Change.(anonymous function) [as wrapBlockByKey] (http://localhost:3000/static/js/bundle.js:202161:15)
    at Object.normalize (http://localhost:3000/static/js/bundle.js:226701:100)
    at http://localhost:3000/static/js/bundle.js:207420:34
    at iterate (http://localhost:3000/static/js/bundle.js:201084:5)
    at normalizeNode (http://localhost:3000/static/js/bundle.js:201105:3)
    at normalizeNodeAndChildren (http://localhost:3000/static/js/bundle.js:201047:5)
    at _loop (http://localhost:3000/static/js/bundle.js:201020:7)
    at normalizeNodeAndChildren (http://localhost:3000/static/js/bundle.js:201042:5)
    at ./node_modules/slate/lib/changes/with-schema.js.Changes.normalizeNodeByKey (http://localhost:3000/static/js/bundle.js:200975:3)
    at Change.call (http://localhost:3000/static/js/bundle.js:202102:10)
    at Change.(anonymous function) [as normalizeNodeByKey] (http://localhost:3000/static/js/bundle.js:202161:15)

This is a blocker for some of the more complex operations (most of them) in my schema rules. I can't seem to find a way around it. Maybe validateNode is best for now on the stuff that may trigger this, until there's a way to prevent the extra normalization?

I'm not sure how I feel about needing to pass options.normalize: false on every change method (could get redundant). Feels kind of clunky compared to the rest of the lib, especially when chaining lots of methods. Maybe the change object itself could be callable and take a normalize option, then return a new change object internally configured not to normalize when its change methods are called?

So instead of:

change
  .removeTextByKey(textNode.key, 0, length, { normalize: false })
  .setNodeByKey(node.key, 'title', { normalize: false })
  .wrapBlockByKey(node.key, 'folder', { normalize: false }); 

It could be:

change({ normalize: false })
  .removeTextByKey(textNode.key, 0, length)
  .setNodeByKey(node.key, 'title')
  .wrapBlockByKey(node.key, 'folder'); 

But, then, in that case is it not mutating the change object that ultimately gets applied? If it has to be the original change throughout the rule, maybe something semantically along the lines of:

change.pauseNormalization();
change
  .removeTextByKey(textNode.key, 0, length)
  .setNodeByKey(node.key, 'title')
  .wrapBlockByKey(node.key, 'folder');
change.resumeNormalization();

That almost looks like a context manager. Is there syntax for that in js?

It definitely wouldn't feel too magical to me to have normalization always disabled inside schema rules, if there were a way to still have the nodes undergo internal normalization after the fact. Is that possible? Maybe there would be a way to collect keys where normalizeNodeByKey was called within a schema rule, then attempt to normalize those keys after the rule normalization is finished? Not sure if that could get too weird with the rule normalization already having performed its own changes.

[alanchrt]

Also, fwiw, this rule seems fine when typing text manually, but fails with the above trace on copy/paste of the same text. Maybe it's actually multiple normalizations bumping into one another? They're synchronous, though, right?

[jasonphillips]

I'm dealing with this issue as well, in this case when a node my rule deleted attempts a new validation.

Update: after carefully ensuring that the normalize:false flag was set properly for any changes I make during normalization -- and then also thinking carefully about the order of invalidations, where invalid child etc have to be thought of in the order in which they'll be called -- I managed to completely avoid this issue in my tests; but again, perhaps an easier way to manage changes without sub-normalizations would help, like that proposed below.

[CameronAckermanSEL]

@jasonphillips I'm also hoping #1549 will help alleviate this problem

[tashburn]

I think I'm experiencing a similar problem.

My validateNode inserts a node at the end of the document --

      validateNode(node) {
        if (node.object != 'document') return
        return change => {
          change.withoutNormalization(change => {
            change.collapseToEndOf(node)
            change.insertBlock(block)
          })
        }
      }

I get infinite recursion --

validateNode call 1: the passed node needs fixing, so I return a change function
validateNode call 2: the node is fixed, so it returns undefined
validateNode call 3: a stale node is passed that doesn't have the fix! so I return yet another fix
validateNode call 4: the node is fixed, so it returns undefined
validateNode call 5: a stale node is passed that doesn't have the fix! so I return yet another fix
... and so on

Am I doing something wrong, or is this a bug?

[DamareYoh]

Hey @tashburn, try

validateNode(node) {
  if (node.object !== 'document) return
  return change => {
    change.withoutNormalization(c => {
      c.collapseToEndOf(node)
      c.insertBlock(block)
    }
  }
}

Using change within the withoutNormalization call may be using the change object that doesnt have the suppressed normalization.

[DamareYoh]

@tashburn upon closer inspection, I think this rule will trigger every single time. I suspect the more accurate way to phrase the purpose of the rule is "ensures that an empty block will be at the end of the document", which means that there should also be an early return that checks if there is already an empty block at the end of the document.

if (node.nodes.get(node.nodes.length -1).isEmpty) return (or something like that)

If the validateNode function returns a change, it signals to the normalizer that there is still something wrong with the document state that needs to be fixed, so it'll keep revisiting the node for normalization until no change functions are returned by validateNode rules.

There's also a way to model this with a schema last rule as an alternative but this is probably fine.

[tashburn]

@DamareYoh Thanks for your thoughts. I actually didn't include some block-type checking code in what I transcribed above. The above validation only ran if the last node wasn't an empty paragraph. It's my error, not a problem with Slate.

Turns out it was recursing because my schema was rejecting the added ending paragraph. I didn't realize that the order of the nodes in schema.document.nodes is enforced in the document.

It might be useful for debugging if we could turn on some sort of logging for when Slate rejects a validation because of the schema.

[DamareYoh]

Schema.document.nodes does not enforce order though. Do you have a jsfiddle that reproduces the behavior?

[tashburn]

@DamareYoh Oh, then there must have been some other schema enforcement going on that prevented my validateNode from working I expected.

I had multiple Schema.document.nodes, each with their own type. When I switched to one Schema.document.node, with multiple types, it worked.

What does it indicate in the schema to have multiple Schema.document.nodes ?

[DamareYoh]

Could we get a snippet of your entire schema? I think that'd add a lot of clarity to the discussion.

[tashburn]

Sure. The old schema that didn't work was --

{
  "document": {
    "nodes": [
      { "types": [ "list-item" ] },
      { "types": [ "paragraph" ] },
      { "types": [ "block-quote" ] },
      { "types": [ "block-code" ] },
      { "types": [ "media" ] },
      { "types": [ "heading1" ] },
      { "types": [ "heading2" ] },
    ]
  },
  "blocks": {
    "list-item": { "nodes": [ { "objects": [ "text", "inline" ] } ] },
    "paragraph": { "nodes": [ { "objects": [ "text", "inline" ] } ] },
    "block-quote": { "nodes": [ { "objects": [ "text", "inline" ] } ] },
    "block-code": { "nodes": [ { "objects": [ "text", "inline" ] } ] },
    "media": { "nodes": [ { "objects": [ "text", "inline" ] } ] },
    "heading1": { "nodes": [ { "objects": [ "text", "inline" ] } ] },
    "heading2": { "nodes": [ { "objects": [ "text", "inline" ] } ] }
  },
  "inlines": {
    "link": { "nodes": [ { "objects": [ "text" ] } ] },
    "reference": { "nodes": [ { "objects": [ "text" ] } ] }
  }
}

And the new schema that works --

{
  "document": {
    "nodes": [
      {
        "types": [
          "list-item",
          "paragraph",
          "block-quote",
          "block-code",
          "media",
          "heading1",
          "heading2"
        ]
      }
    ]
  },
  "blocks": {
    "list-item": { "nodes": [ { "objects": [ "text", "inline" ] } ] },
    "paragraph": { "nodes": [ { "objects": [ "text", "inline" ] } ] },
    "block-quote": { "nodes": [ { "objects": [ "text", "inline" ] } ] },
    "block-code": { "nodes": [ { "objects": [ "text", "inline" ] } ] },
    "media": { "nodes": [ { "objects": [ "text", "inline" ] } ] },
    "heading1": { "nodes": [ { "objects": [ "text", "inline" ] } ] },
    "heading2": { "nodes": [ { "objects": [ "text", "inline" ] } ] }
  },
  "inlines": {
    "link": { "nodes": [ { "objects": [ "text" ] } ] },
    "reference": { "nodes": [ { "objects": [ "text" ] } ] }
  }
} 

I'm manually combining the schemas from plugins and feeding one schema into the Editor, because the built-in plugin schema composition seems to have a bug.

Also, here's my unadulterated validateNode feature, if that helps --

function ValidateLastNodeEmpty(type:string, blockValue:object): Feature {
  return featurize({
    validateNode(node:Node) {
      if (node.object != 'document') return
      const doc = node as Document
      let lastNode = doc.nodes.get(doc.nodes.size - 1) // might be undefined
      let lastNodeIsTargetType = lastNode && lastNode.type == type
      let lastNodeIsEmpty = !lastNode || lastNode.text.length == 0
      if (!lastNode || !lastNodeIsTargetType || (lastNodeIsTargetType && !lastNodeIsEmpty)) {
        return change => {
          return change.withoutNormalization(change => {
            const block = Block.create(blockValue)
            change.insertNodeByKey(doc.key, doc.nodes.size, block)
          })
        }
      }
    }
  })
}

[zhujinxuan]

Hi, perhaps this PR may help to fix the problem. https://github.com/ianstormtaylor/slate/pull/1805

It is use a sequence to normalizeNodeByKey, if a normalizeNodeByKey task is running, then do not do interrupt the task.

[ianstormtaylor]

This can be solved by using deferNormalization, in combination with improvements to the performance from https://github.com/ianstormtaylor/slate/issues/2136.