Slate: add `options.save` to all changes

[ianstormtaylor]

This is an idea. By passing options down in all of the change methods, like @doodlewind did in setValue() it would allow us to have any change not be saved by doing:

change.someMethod(...args, { save: false })

Which seems nicer than the current confusing setFlag('save', false)...setFlag('save', true) workflow surrounding changes. It would probably allow us to remove that weird code.

[doodlewind]

setValue is not documented nor publicly exposed, so adding save: false is just fine.

I'd vote for it, as for the side effect, I believe we removed transform.someMethod(...args, { save: false }) in v0.22 to fully support potential OT, keeping all operations information tracked. If we add this back, can we still preserve our history stack intact?

[ianstormtaylor]

@doodlewind setValue is publicly exposed.

What do you mean about the side effects? The change I'm proposing is just allowing for options.save, so that people can customize, but still defaulting to save: true to save things into the history. I might not have been clear about that.

[doodlewind]

Ah my fault 😳 I can think about some scenarios for this API.

Say we have an editor supports attachment upload, during uploading we insert a custom block with progress bar updated realtime, and after upload is done we replace it. So IMO what user expects is:

1. During uploading, cmd-z should directly removes the progress bar.
2. After upload is done, cmd-z should also directly removes the attachment block.
3. cmd-z shouldn't change the state of progress bar.

So it makes sense to { save: false } for all the changes that merely updating progress bar, and this is simpler than the setFlag('save', true) approach. Let's make this happen!

[doodlewind]

Well I came up with idea with another approach. In the use case mentioned above, what we { save: false } is indeed a state without the need to be serialized. Adding the progress bar state information into node.data is somewhat overkill itself.

So how about this idea? We add a state field into node, so then we can update state via Slate's change instead of React's setState, but we always not saving node.state into history, neither do we serialize it. In this way we can always keep the escape hatch closed.

[ianstormtaylor]

I think we're going to need the save: false regardless of that one example.

But I do agree that for that example, the progress updates probably should not be modeled as changes to the document. Instead, I'd recommend they be handled internally to the React component in the render layer only. But the final src = ... should use save: false for the same reason as you mentioned previously.

I'd rather not have node.state, and instead just have this handled in React-land I think.

[chemzqm]

+1 for {save: false} option, there was a {save: false} option in slate 0.21 and it works well.

[ianstormtaylor]

To be clear, this would add { save: false } as an optional argument to all change methods, such that you can write:

change.addMarkByKey(node.key, 'bold', { save: false })

But if you do multiple changes, you'll need to add { save: false } to each of them.

[tobiasandersen]

This would be really handy! I can make a PR if you still want it @ianstormtaylor?

[ianstormtaylor]

@tobiasandersen that would be great!

[ianstormtaylor]

This is solved now by change.withoutSaving().