Autosave improvements #10218
Autosave improvements #10218
Conversation
Fix (autosave): Autosave callback should not be called while the editor is initialized.
| // Change may not produce an operation, so the document's version | ||
| // can be the same after that change. | ||
| if ( | ||
| version < this._lastDocumentVersion || |
There was a problem hiding this comment.
It seems to me that this condition can never be met. At most version can be the same as _lastDocumentVersion but it can never be lower, as _lastDocumentVersion is set to it, and document version never goes down.
There was a problem hiding this comment.
Yup, as I thought it was at some point <= (which AFAIU also should simply be ==) and it was changed to < because of markers that affect data but does not bump version: 5577aa2#diff-f3cd7394728f0a6168a108ff56dc6ee631d0a71eb34f1a762264f9339de28311L161
But we don't need to check versions anyway -- _save() is called (debounced) only in change:date events, so we can be sure that there was actual change.
There was a problem hiding this comment.
BTW. TBH I don't know what was the use case but if a marker affects data it should also use operations 🤔 .
While use-operation, not-data may make sense, the other combination does not. If marker affects data it has to be send over to other clients.
There was a problem hiding this comment.
However, we need to check version lower, in the promise resolution:
if ( this.editor.model.document.version > this._lastDocumentVersion ) {
And there might be a problem there with the markers that don't bump version but:
- This change (removing
if) is not related to the code below. - As I explained, I am not sure what kind of marker affects data but does not use operations 🤔 . It may be a good idea to try and find that as a part of this PR review.
| // can be the same after that change. | ||
| if ( | ||
| version < this._lastDocumentVersion || | ||
| this.editor.state === 'initializing' |
There was a problem hiding this comment.
This is wrong assumption. Data change might be triggered during editor initialization (when editor state is initializing) but _save() might be actually called later (with delay) when the editor state is already ready.
| } ) | ||
| .then( _editor => { | ||
| editor = _editor; | ||
|
|
||
| autosave = editor.plugins.get( Autosave ); | ||
|
|
||
| const data = '<p>paragraph1</p><p>paragraph2</p>'; |
There was a problem hiding this comment.
Moved data initialization where it should be. Didn't have any impact on tests.
| @@ -25,14 +33,6 @@ ClassicEditor | |||
| destroyButton.addEventListener( 'click', () => editor.destroy() ); | |||
|
|
|||
| const autosave = editor.plugins.get( Autosave ); | |||
| autosave.adapter = { | |||
There was a problem hiding this comment.
Used config callback instead of adapter because there is no word about adapter anywhere in our docs or guides, it is just mentioned somewhere deeper in API docs. I believe this is forgotten/not recommended way to integrate autosave.
Funny, with this integration, the problem with calling autosave during initialization does not happen (because the adapter is added after the editor is initialized).
Suggested merge commit message (convention)
Fix (autosave): Autosave callback should not be called while the editor is initialized. Closes #10214.
Feature (autosave): Introduced `Autosave#save()`. Closes #10215.