Fixed-schedule AutosaveMonitor (#23962)

* Initial stab at throttled, hook-based AutosaveMonitor * Pass all tests * Simplify the logic and refactor use-scheduled-save to use-throttle * Further simplify useThrottle * Further simplify useThrottle * Simplify AutosaveMonitor * Fixed-schedule based AutosaveMonitor * Improve naming of things * Remove obsolete shouldThrottle from AutosaveMonitor * Roll back obsolete changes * Revisit some of the unit tests for AutosaveMonitor * Adjust unit tests * Fix a few flaky autosave e2e tests * Fix a few flaky autosave e2e tests * Update sessionStorage key used in e2e tests * Adjust the e2e test to throttle-less timer
WordPress · Jul 30, 2020 · 6cce6e0 · 6cce6e0
1 parent a7bf428
commit 6cce6e0
Show file tree

Hide file tree

Showing 5 changed files with 137 additions and 141 deletions.
diff --git a/packages/core-data/src/selectors.js b/packages/core-data/src/selectors.js
@@ -660,5 +660,9 @@ export const hasFetchedAutosaves = createRegistrySelector(
  */
 export const getReferenceByDistinctEdits = createSelector(
 	() => [],
-	( state ) => [ state.undo.length, state.undo.offset ]
+	( state ) => [
+		state.undo.length,
+		state.undo.offset,
+		state.undo.flattenedUndo,
+	]
 );
diff --git a/packages/e2e-tests/specs/editor/various/autosave.test.js b/packages/e2e-tests/specs/editor/various/autosave.test.js
@@ -44,7 +44,7 @@ async function clearSessionStorage() {
 async function readSessionStorageAutosave( postId ) {
 	return page.evaluate(
 		( key ) => window.sessionStorage.getItem( key ),
-		`wp-autosave-block-editor-post-${ postId }`
+		`wp-autosave-block-editor-post-${ postId ? postId : 'auto-draft' }`
 	);
 }
 
@@ -76,9 +76,14 @@ describe( 'autosave', () => {
 	} );
 
 	it( 'should save to sessionStorage', async () => {
+		// Wait for the original timeout to kick in, it will schedule
+		// another run using the updated interval length of AUTOSAVE_INTERVAL_SECONDS
+		await sleep( 15 );
+
 		await clickBlockAppender();
 		await page.keyboard.type( 'before save' );
 		await saveDraftWithKeyboard();
+		await sleep( 1 );
 		await page.keyboard.type( ' after save' );
 
 		// Wait long enough for local autosave to kick in
@@ -88,19 +93,6 @@ describe( 'autosave', () => {
 		const autosave = await readSessionStorageAutosave( id );
 		const { content } = JSON.parse( autosave );
 		expect( content ).toBe( wrapParagraph( 'before save after save' ) );
-
-		// Test throttling by scattering typing
-		await page.keyboard.type( ' 1' );
-		await sleep( AUTOSAVE_INTERVAL_SECONDS - 4 );
-		await page.keyboard.type( '2' );
-		await sleep( 2 );
-		await page.keyboard.type( '3' );
-		await sleep( 2 );
-
-		const newAutosave = await readSessionStorageAutosave( id );
-		expect( JSON.parse( newAutosave ).content ).toBe(
-			wrapParagraph( 'before save after save 123' )
-		);
 	} );
 
 	it( 'should recover from sessionStorage', async () => {
@@ -219,7 +211,7 @@ describe( 'autosave', () => {
 		);
 		expect(
 			await page.evaluate( () => window.sessionStorage.length )
-		).toBe( 1 );
+		).toBeGreaterThanOrEqual( 1 );
 
 		// Trigger remote autosave
 		await page.evaluate( () =>
@@ -295,14 +287,14 @@ describe( 'autosave', () => {
 		);
 		expect(
 			await page.evaluate( () => window.sessionStorage.length )
-		).toBe( 1 );
+		).toBeGreaterThanOrEqual( 1 );
 
 		// Bring network down and attempt to save
 		toggleOfflineMode( true );
 		saveDraftWithKeyboard();
 		expect(
 			await page.evaluate( () => window.sessionStorage.length )
-		).toBe( 1 );
+		).toBeGreaterThanOrEqual( 1 );
 	} );
 
 	it( "shouldn't conflict with server-side autosave", async () => {
@@ -326,7 +318,7 @@ describe( 'autosave', () => {
 		);
 		expect(
 			await page.evaluate( () => window.sessionStorage.length )
-		).toBe( 1 );
+		).toBeGreaterThanOrEqual( 1 );
 
 		await page.reload();
 

diff --git a/packages/editor/src/components/autosave-monitor/index.js b/packages/editor/src/components/autosave-monitor/index.js
@@ -6,63 +6,53 @@ import { compose } from '@wordpress/compose';
 import { withSelect, withDispatch } from '@wordpress/data';
 
 export class AutosaveMonitor extends Component {
-	componentDidUpdate( prevProps ) {
-		const {
-			isDirty,
-			editsReference,
-			isAutosaveable,
-			isAutosaving,
-		} = this.props;
+	constructor( props ) {
+		super( props );
+		this.needsAutosave = !! ( props.isDirty && props.isAutosaveable );
+	}
 
-		// The edits reference is held for comparison to avoid scheduling an
-		// autosave if an edit has not been made since the last autosave
-		// completion. This is assigned when the autosave completes, and reset
-		// when an edit occurs.
-		//
-		// See: https://github.com/WordPress/gutenberg/issues/12318
+	componentDidMount() {
+		this.setAutosaveTimer();
+	}
 
-		if ( editsReference !== prevProps.editsReference ) {
-			this.didAutosaveForEditsReference = false;
+	componentDidUpdate( prevProps ) {
+		if ( ! this.props.isDirty && prevProps.isDirty ) {
+			this.needsAutosave = false;
+			return;
 		}
 
-		if ( ! isAutosaving && prevProps.isAutosaving ) {
-			this.didAutosaveForEditsReference = true;
+		if ( this.props.isAutosaving && ! prevProps.isAutosaving ) {
+			this.needsAutosave = false;
+			return;
 		}
 
-		if (
-			prevProps.isDirty !== isDirty ||
-			prevProps.isAutosaveable !== isAutosaveable ||
-			prevProps.editsReference !== editsReference
-		) {
-			this.toggleTimer(
-				isDirty && isAutosaveable && ! this.didAutosaveForEditsReference
-			);
+		if ( this.props.editsReference !== prevProps.editsReference ) {
+			this.needsAutosave = true;
 		}
 	}
 
 	componentWillUnmount() {
-		this.toggleTimer( false );
+		clearTimeout( this.timerId );
 	}
 
-	toggleTimer( isPendingSave ) {
-		const { interval, shouldThrottle = false } = this.props;
+	setAutosaveTimer( timeout = this.props.interval * 1000 ) {
+		this.timerId = setTimeout( () => {
+			this.autosaveTimerHandler();
+		}, timeout );
+	}
 
-		// By default, AutosaveMonitor will wait for a pause in editing before
-		// autosaving. In other words, its action is "debounced".
-		//
-		// The `shouldThrottle` props allows overriding this behaviour, thus
-		// making the autosave action "throttled".
-		if ( ! shouldThrottle && this.pendingSave ) {
-			clearTimeout( this.pendingSave );
-			delete this.pendingSave;
+	autosaveTimerHandler() {
+		if ( ! this.props.isAutosaveable ) {
+			this.setAutosaveTimer( 1000 );
+			return;
 		}
 
-		if ( isPendingSave && ! ( shouldThrottle && this.pendingSave ) ) {
-			this.pendingSave = setTimeout( () => {
-				this.props.autosave();
-				delete this.pendingSave;
-			}, interval * 1000 );
+		if ( this.needsAutosave ) {
+			this.needsAutosave = false;
+			this.props.autosave();
 		}
+
+		this.setAutosaveTimer();
 	}
 
 	render() {
@@ -84,9 +74,9 @@ export default compose( [
 		const { interval = getEditorSettings().autosaveInterval } = ownProps;
 
 		return {
+			editsReference: getReferenceByDistinctEdits(),
 			isDirty: isEditedPostDirty(),
 			isAutosaveable: isEditedPostAutosaveable(),
-			editsReference: getReferenceByDistinctEdits(),
 			isAutosaving: isAutosavingPost(),
 			interval,
 		};