Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Save callback should not be executed while the previous wasn't resolved #2539

Closed
Reinmar opened this issue Jul 13, 2018 · 3 comments · Fixed by ckeditor/ckeditor5-autosave#16
Assignees
Labels
package:autosave status:discussion type:bug This issue reports a buggy (incorrect) behavior.
Milestone

Comments

@Reinmar
Copy link
Member

Reinmar commented Jul 13, 2018

I wrote such a simple sample:

/**
 * @license Copyright (c) 2003-2018, CKSource - Frederico Knabben. All rights reserved.
 * For licensing, see LICENSE.md.
 */

/* globals ClassicEditor, console, window, document, setTimeout */

import { CS_CONFIG } from '@ckeditor/ckeditor5-cloud-services/tests/_utils/cloud-services-config';

ClassicEditor
	.create( document.querySelector( '#snippet-autosave' ), {
		cloudServices: CS_CONFIG,
		toolbar: {
			viewportTopOffset: 60
		},
		autosave: {
			save( editor ) {
				return saveData( editor.getData() );
			}
		}
	} )
	.then( editor => {
		window.editor = editor;
	} )
	.catch( err => {
		console.error( err.stack );
	} );

function saveData( data ) {
	return new Promise( resolve => {
		log( `Saving... (${ data })` );

		setTimeout( () => {
			log( 'Saved.' );

			resolve();
		}, 1500 );
	} );
}

function log( msg ) {
	const console = document.querySelector( '#snippet-autosave-console' );

	console.textContent = msg + '\n' + console.textContent;
}
<div id="snippet-autosave">
	<p>TODO</p>
</div>

<pre id="snippet-autosave-console"></pre>

It behaves like this now:

jul-13-2018 15-29-46

IMO, it should wait with the next "Saving..." until the previous save attempt was completed.

It isn't a big deal if it doesn't wait, but it's odd. It wasn't natural for me.

@Reinmar
Copy link
Member Author

Reinmar commented Jul 13, 2018

Adding to the next iteration, but it'd be good to get some feedback what's the expected behaviour. We can also check some other implementations.

@wwalc
Copy link
Member

wwalc commented Jul 23, 2018

it should wait with the next "Saving..." until the previous save attempt was completed.

Just a note that if we ever implement this, we should handle correctly a situation where the first save attempt failed (not sure what was the definition of "completed" above). We should try to continue saving the data again after getting an error/timeout or sth.

@pjasiun
Copy link

pjasiun commented Jul 26, 2018

Just a note that if we ever implement this, we should handle correctly a situation where the first save attempt failed (not sure what was the definition of "completed" above). We should try to continue saving the data again after getting an error/timeout or sth.

We agreed to handle errors in the separate ticket. In this ticket, we should focus not to execute another save action before the previous one finish.

@ma2ciek ma2ciek self-assigned this Jul 27, 2018
pjasiun referenced this issue in ckeditor/ckeditor5-autosave Aug 16, 2018
Other: Improved call frequency. Closes #9. Closes #10. Closes #12. Closes ckeditor/ckeditor5#1158.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-autosave Oct 9, 2019
@mlewand mlewand added this to the iteration 20 milestone Oct 9, 2019
@mlewand mlewand added status:discussion type:bug This issue reports a buggy (incorrect) behavior. package:autosave labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:autosave status:discussion type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants