Slate: onDocumentChange not called on block type change

[oyeanuj]

Hi @ianstormtaylor! I notice that if the block type changes (without any change to the text), the onDocumentChange is not being fired. Is that expected?

Intuitively, I would expect that a change in block type would be considered a material change in document, to invoke this event.

Here is a JSFiddle demonstrating this - https://jsfiddle.net/urr1z99g/1/

Steps to Reproduce

1. Open console.
2. Click on 'Toggle Heading' to start a heading.
3. At this point, I would expect the onDocumentChange and onChange to be fired.

Thank you!

[oyeanuj]

@ianstormtaylor I can still reproduce the problem in the above example with the above commit.

[oyeanuj]

moreover, onChange is also not being fired on the change in block type. _(Updated the url of the fiddle to reflect that)._

[tyler-johnson]

We are experiencing this, too. Seems to happen with all of our "outside" transforms, anything we call editor.onChange() for.

I have found that onBeforeChange() is still firing, though.

[vpontis]

So I took a little look at this problem. Yea, it does look like it's a problem w/ all transformations that are called via Javascript.

onBeforeChange is called on Editor.componentWillReceiveProps. But onChange is called as a proxy through the EVENT_HANDLERS.

So here are some options:

• Add a custom EVENT_HANDLER for the transformations
• Somehow propagate onChange when in applyTransform
• Call onChange in the componentWillReceiveProps

@ianstormtaylor, what do you think? I'm still learning how everything is architected so I don't have an opinion yet.

[tommoor]

This is a pretty major issue for persistence. It would be great to get some pointers on how to fix this up if you don't have time to implement a fix yourself @ianstormtaylor

[Mangatt]

I've run into this issue as well. My plugin uses onKeyDown events as well as external buttons to trigger that action. Thing is that keyboard shortcut works fine, but button does not work.

Could this possibly be linked to losing focus of editor? I mean that onDocumentChange is triggered only with focused editor?

[erquhart]

Here's a workaround for now:

1. Keep using onDocumentChange to avoid needless updates.
2. For externally initiated transforms (via buttons, keydown, etc), call the onChange method from your Editor instance directly, and pass in the transformed state.
3. Call setState after calling onChange.

Example:

handler = () => {
  const resolvedState = this.state.state.transform().setBlock('header-one').apply();
  this.ref.onChange(resolvedState);
  this.setState({ state: resolvedState });
}

This did not result in any additional renders of the Editor component in my testing.

@ianstormtaylor I did spend a fair amount of time looking into a proper fix, but this seems to go pretty deep into Slate's update lifecycle, and it'll take more time to figure out what's happening when (and why), and ensuring that a fix doesn't cause regressions. This workaround should at least take this issue out of blocker territory for most folks.

[ianstormtaylor]

This issue hasn't had a response in a while, and it's unclear if it's still present in the latest versions of Slate. To keep the issues easier to manage, since there are so many being opened every day, I'm going to close this old one out.

But if you notice that the bug is still happening, please feel free to comment and it can be revisited. It might be an edge case that not many people run into, in which case the fastest way to get it fixed would probably be to write a pull request for it.

Thanks for understanding!

[czechdave]

This issue is still present. I'm currently working around it by using onChange and manually comparing the serialized state to detect changes...

[oyeanuj]

@ianstormtaylor I can also confirm that this still exists in the latest version, and should affect anyone who is using onDocumentChange.

@czechdave Could you share your solution here - I'm curious how your method differs from the core code.

[ianstormtaylor]

Sounds good, reopening. Someone will need to pull request a fix for this—I either am doing something different with my own onDocumentChange and missing this, or I just haven't run into this at all.

[oyeanuj]

@ianstormtaylor Interesting. I noticed it when dealing with toggling of blocks. Are you able to reproduce this super simplified JSFiddle - https://jsfiddle.net/urr1z99g/1/ ?

[ianstormtaylor]

@oyeanuj yup I can reproduce it there.

[czechdave]

Ok, so I've found out why this issue is happening but not sure how to resolve it as I'm not as familiar with the core code.

The places to look at are Editor.componentWillReceiveProps and Editor.onChange
https://github.com/ianstormtaylor/slate/blob/master/src/components/editor.js#L145
https://github.com/ianstormtaylor/slate/blob/master/src/components/editor.js#L227

The docs say that setState and calling Editor.onChange are the same but that's not true. When we do setState outside of the Editor then the Editor.componentWillReceiveProps runs and never executes the logic within Editor.onChange that is responsible for triggering onDocumentChange...

[tobiasandersen]

As described by @czechdave, this problem only occurs if you pass Slate a state that you created on your own, outside of Slate. But if we view Slate as a black box that notifies us when things inside it change, I think this behavior actually makes sense. Take these scenarios:

Scenario 1:
The Enter key was pressed by a user, Slate takes care of this by splitting the current block in two, and then notifies us about the changes (by calling both props.onChange and props.onDocumentChange). _Not that it does not yet apply those changes._ We get the changes, save the content to the db, and then we give the updated state back to Slate. This is when the changes are applied. All good.

Scenario 2:
A user presses a toolbar button for turning blocks into comments. Slate has no idea about this button, so it doesn't do anything. But we know about this button, so we create a new state with that block toggled to a comment. Now we give that state back to Slate, and the changes get applied. Everything looks alright, but notice that we haven't saved the changes to our db.

Ok, so how do we solve this?
If we look at the first scenario, we see that we save our changes to the db before we actually give that state back to Slate. If we were to do the same in the second scenario, the time for saving things to db would be when the button was pressed. That is, even before Slate knows about anything.

If we agree this is how it should work, then it'd be our responsibility to call our onDocumentChange handler when we change things. And isn't that how it should work? When Slate change stuff, Slate calls the handlers — when we change things, we call the handlers.

If we don't agree on this, we can indeed make Slate call our onDocumentChange when it receives a state with a document that it hasn't created on its own. It would only need to track whether or not the state was created inside or outside of Slate. I could make a PR for this — but I'm not sure it's a good idea? @ianstormtaylor

[ianstormtaylor]

@tobiasandersen thank you! Extremely well explained.

I think I agree with you, this seems like a case where Slate shouldn't actually be doing anything, because it would break the pattern that most React components setup.

That said, this is very confusing from a UX perspective, judging by the popularity of this issue. Given that people usually setup their code like this:

class Component {
  onChange = ({ state }) => { ... }
  onDocumentChange = ({ state }) => { ... }
  render() { ... }
}

And are likely to be "updating" the state manually in Scenario 2 like you described like so:

this.onChange(change)

I think we might be able to solve the confusion by deprecating onDocumentChange. This would cause people to have to keep track of it themselves in their onChange handler. And when they went to manually update the state by calling this.onChange they'd get the save functionality still. Seems like it avoids the current confusing piece where calling this.onChange doesn't result in this.onDocumentChange being called.

What do you think?

[tobiasandersen]

I agree, should work if everyone just did something like this instead in their onChange-handler:

onChange = (change) => {
  const { state } = change

  if (state.document !== this.state.state.document) {
    this.onDocumentChange(state.document, change)
  }

  this.setState({ state })
}

However, I continued this discussion a bit with @YurkaninRyan on Slack, and he brought up another similar point: What happens with my validation rules if I pass Slate a state that I've created?

[ianstormtaylor]

@tobiasandersen we used to try to call onBeforeChange inside the componentWillReceiveProps, but IMO it led to weird behavior. I think it's another case where it's not the "React way" to be changing the props that get passed into the Editor when they are passed in.

Potentially the validation rule issue would be solved by making schema a part of state. Such that when changing a state, you could guarantee that schema rules validation was incorporated. I'm definitely open to this.

Does that solve the issue? Or are you seeing something I'm missing? (Very possible 😄 )

[oyeanuj]

@tobiasandersen @ianstormtaylor Interesting points. As one of those confused by the current UX, I will try to explain why (assuming we haven't already made the decision to deprecate it):

I think of onDocumentChange, very simplistically, as an event that should be fired when the document is changed. It can be changed by the user, natively or by a state change that we pass. I think if it was onDocumentChangeByNativeAction, maybe we'd think very differently about it. I think the saving to the DB is just a side effect, a red herring in this discussion.

I've always thought of this method as a child/younger sibling of, or at the very least related to the onChange method. So, the expectation has always been that Slate or we call the onChange method whenever there is a new state. And if then within that state, there is a change in the document, then onDocumentChange would be called.

So, in other words the mental model that seems the most seamless,

1. Change occurs (wherever)
2. Slate's onChange is called (by us or internal Slate methods).
3. If document is changed, call onDocumentChange.
4. Call onChange prop passed to the Editor.

If you agree to that the model, then in Scenario 2, the flow is broken because step 3 is skipped and pushed to userland but step 4 still occurs.

Thoughts?

[ianstormtaylor]

@oyeanuj I think the issue is that with the mental model, for Step 2 people actually aren't calling Slate's onChange (which would be editor.onChange).

Instead, for cases where you're updating manually instead of Slate's internal updates, you're actually calling this.onChange, which is the parent of the editor. And the editor never knows about it, it just ends up receiving a new state via the parent eventually doing this.setState in the this.onChange handler.

I think onDocumentChange (and onSelectionChange) are harmful because they feel like event handlers that will always fire, even when calling setState. Whereas, it's much more clear why onChange doesn't fire when you call setState, because that would cause an infinite loop. And that expectation is set more clearly by all other React components too.

I think deprecating onDocumentChange and onSelectionChange will clear this up a lot.

[oyeanuj]

@ianstormtaylor Not sure about this point above

for Step 2 people actually aren't calling Slate's onChange (which would be editor.onChange).

In my usecase, and @tyler-johnson's usecase above, we are calling editor.onChange and not just setState or this.onChange.

So, it might be that this issue is occuring in both of these cases, which means that there is another Scenario 3 to the case @tobiasandersen mentioned above:

Scenario 3:
A user presses a toolbar button for turning blocks into comments. Slate has no idea about this button, so it doesn't do anything. But we know about this button, so we create a new state with that block toggled to a comment. Now we give that state back to Slate via editor.onChange, and the changes get applied.

So, in this case, the mental model is valid, right? And I think in the case of setState, it is intuitive why they don't fire because of the infinite loops.

So, to throw out the functions to clarify one scenario seems like over-remedying it but I could see this being a plugin that once implemented can be re-used. Would having it in the core be more optimized for any reason?

[ianstormtaylor]

@oyeanuj not sure exactly if I follow you. In the JSFiddle in the opening comment of the issue, it looks like onClickBlock eventually calls this.setState instead of editor.onChange?

[Slapbox]

What's the preferred alternative to onDocumentChange? Any suggestions? I thought it was simple enough, but @oyeanuj makes me question if I'm thinking it through enough.

[YurkaninRyan]

You can just use onChange and shallow compare the previous document to the incoming document, the you can fire your document change handler

[Slapbox]

@YurkaninRyan Thanks a lot. I'm using Electron which has made becoming comfortable with Immutable much harder since I'm relatively new to programming and I can't use the nifty devtools in Electron.

[YurkaninRyan]

@Slapbox no problem my friend :). Join us on the slack if you have anymore questions or want to brainstorm