From 6cce6e0fdbb27d496370d48305913523e3251ea1 Mon Sep 17 00:00:00 2001 From: Adam Zielinski Date: Thu, 30 Jul 2020 11:13:49 +0200 Subject: [PATCH] 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 --- packages/core-data/src/selectors.js | 6 +- .../specs/editor/various/autosave.test.js | 28 ++- .../src/components/autosave-monitor/index.js | 74 ++++---- .../components/autosave-monitor/test/index.js | 169 ++++++++++-------- .../local-autosave-monitor/index.js | 1 - 5 files changed, 137 insertions(+), 141 deletions(-) diff --git a/packages/core-data/src/selectors.js b/packages/core-data/src/selectors.js index f17c9a2bd7d2d..932ab6bbcd061 100644 --- 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 index 5780302794aac..b2f682839d9b3 100644 --- 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 index 1cc03a6e39ecb..938b8b16206c9 100644 --- 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, }; diff --git a/packages/editor/src/components/autosave-monitor/test/index.js b/packages/editor/src/components/autosave-monitor/test/index.js index 68c177171f161..5ccf70d182dd5 100644 --- a/packages/editor/src/components/autosave-monitor/test/index.js +++ b/packages/editor/src/components/autosave-monitor/test/index.js @@ -9,122 +9,133 @@ import { shallow } from 'enzyme'; import { AutosaveMonitor } from '../'; describe( 'AutosaveMonitor', () => { - const toggleTimer = jest.fn(); let wrapper; + let setAutosaveTimerSpy; beforeEach( () => { - toggleTimer.mockClear(); + jest.useFakeTimers(); + setAutosaveTimerSpy = jest.spyOn( + AutosaveMonitor.prototype, + 'setAutosaveTimer' + ); wrapper = shallow( , { lifecycleExperimental: true, } ); + } ); - wrapper.instance().toggleTimer = toggleTimer; + afterEach( () => { + setAutosaveTimerSpy.mockClear(); } ); - describe( '#componentDidUpdate()', () => { - it( 'should start autosave timer when having become dirty and saveable', () => { - wrapper.setProps( { isDirty: true, isAutosaveable: true } ); + it( 'should start autosave timer after being mounted', () => { + expect( setAutosaveTimerSpy ).toHaveBeenCalled(); + } ); - expect( toggleTimer ).toHaveBeenCalledWith( true ); - } ); + it( 'should clear the autosave timer after being unmounted', () => { + wrapper.unmount(); + expect( clearTimeout ).toHaveBeenCalled(); + } ); - it( 'should restart autosave timer when edits reference changes', () => { - const beforeReference = []; - const afterReference = []; + describe( '#componentDidUpdate()', () => { + it( 'should set needsAutosave=true when editReference changes and other props stay the same (1)', () => { + expect( wrapper.instance().needsAutosave ).toBe( false ); wrapper.setProps( { - isDirty: true, - isAutosaveable: true, - editsReference: beforeReference, + editsReference: [], } ); - toggleTimer.mockClear(); + expect( wrapper.instance().needsAutosave ).toBe( true ); + } ); + it( 'should set needsAutosave=true when editReference changes and the post becomes dirty', () => { + expect( wrapper.instance().needsAutosave ).toBe( false ); wrapper.setProps( { isDirty: true, - isAutosaveable: true, - editsReference: beforeReference, + editsReference: [], } ); + expect( wrapper.instance().needsAutosave ).toBe( true ); + } ); - expect( toggleTimer ).not.toHaveBeenCalled(); - + it( 'should not set needsAutosave=true when editReference changes and the post is not dirty anymore', () => { + expect( wrapper.instance().needsAutosave ).toBe( false ); wrapper.setProps( { isDirty: true, - isAutosaveable: true, - editsReference: afterReference, + editsReference: [], } ); - - expect( toggleTimer ).toHaveBeenCalledWith( true ); - } ); - - it( 'should stop autosave timer when the autosave is up to date', () => { - wrapper.setProps( { isDirty: true, isAutosaveable: false } ); - - expect( toggleTimer ).toHaveBeenCalledWith( false ); - } ); - - it( 'should stop autosave timer when having become dirty but not autosaveable', () => { - wrapper.setProps( { isDirty: true, isAutosaveable: false } ); - - expect( toggleTimer ).toHaveBeenCalledWith( false ); - } ); - - it( 'should stop autosave timer when having become not dirty', () => { - wrapper.setProps( { isDirty: true } ); - toggleTimer.mockClear(); - wrapper.setProps( { isDirty: false } ); - - expect( toggleTimer ).toHaveBeenCalledWith( false ); + wrapper.setProps( { + isDirty: false, + editsReference: [], + } ); + expect( wrapper.instance().needsAutosave ).toBe( false ); } ); - it( 'should stop autosave timer when having become not autosaveable', () => { - wrapper.setProps( { isDirty: true } ); - toggleTimer.mockClear(); - wrapper.setProps( { isAutosaveable: false } ); - - expect( toggleTimer ).toHaveBeenCalledWith( false ); + it( 'should set needsAutosave=true when editReference changes and the post is not autosaving', () => { + expect( wrapper.instance().needsAutosave ).toBe( false ); + wrapper.setProps( { + isAutosaving: false, + editsReference: [], + } ); + expect( wrapper.instance().needsAutosave ).toBe( true ); } ); - it( 'should avoid scheduling autosave if still dirty but already autosaved for edits', () => { - // Explanation: When a published post is autosaved, it's still in a - // dirty state since the edits are not saved to the post until the - // user clicks "Update". To avoid recurring autosaves, ensure that - // an edit has occurred since the last autosave had completed. - - const beforeReference = []; - const afterReference = []; - - // A post is non-dirty while autosave is in-flight. + it( 'should not set needsAutosave=true when editReference changes and the post started autosaving', () => { + expect( wrapper.instance().needsAutosave ).toBe( false ); + wrapper.setProps( { + isAutosaving: false, + editsReference: [], + } ); wrapper.setProps( { - isDirty: false, isAutosaving: true, - isAutosaveable: true, - editsReference: beforeReference, + editsReference: [], } ); - toggleTimer.mockClear(); + expect( wrapper.instance().needsAutosave ).toBe( false ); + } ); + } ); + + describe( '#autosaveTimerHandler()', () => { + it( 'should schedule itself in another {interval} ms', () => { wrapper.setProps( { - isDirty: true, - isAutosaving: false, isAutosaveable: true, - editsReference: beforeReference, + interval: 5, } ); + expect( setAutosaveTimerSpy ).toHaveBeenCalledTimes( 1 ); + wrapper.instance().autosaveTimerHandler(); + expect( setAutosaveTimerSpy ).toHaveBeenCalledTimes( 2 ); + expect( setTimeout ).lastCalledWith( expect.any( Function ), 5000 ); + } ); - expect( toggleTimer ).toHaveBeenCalledWith( false ); + it( 'should schedule itself in 1000 ms if the post is not autosaveable at a time', () => { + wrapper.setProps( { + isAutosaveable: false, + interval: 5, + } ); + expect( setAutosaveTimerSpy ).toHaveBeenCalledTimes( 1 ); + wrapper.instance().autosaveTimerHandler(); + expect( setAutosaveTimerSpy ).toHaveBeenCalledTimes( 2 ); + expect( setTimeout ).lastCalledWith( expect.any( Function ), 1000 ); + } ); - // Once edit occurs after autosave, resume scheduling. + it( 'should call autosave if needsAutosave=true', () => { + const autosave = jest.fn(); wrapper.setProps( { - isDirty: true, - isAutosaving: false, isAutosaveable: true, - editsReference: afterReference, + interval: 5, + autosave, } ); - - expect( toggleTimer.mock.calls[ 1 ][ 0 ] ).toBe( true ); + wrapper.instance().needsAutosave = true; + expect( autosave ).toHaveBeenCalledTimes( 0 ); + wrapper.instance().autosaveTimerHandler(); + expect( autosave ).toHaveBeenCalledTimes( 1 ); } ); - } ); - - describe( '#componentWillUnmount()', () => { - it( 'should stop autosave timer', () => { - wrapper.unmount(); - expect( toggleTimer ).toHaveBeenCalledWith( false ); + it( 'should not call autosave if needsAutosave is not true', () => { + const autosave = jest.fn(); + wrapper.setProps( { + isAutosaveable: true, + interval: 5, + autosave, + } ); + wrapper.instance().needsAutosave = false; + expect( autosave ).toHaveBeenCalledTimes( 0 ); + wrapper.instance().autosaveTimerHandler(); + expect( autosave ).toHaveBeenCalledTimes( 0 ); } ); } ); diff --git a/packages/editor/src/components/local-autosave-monitor/index.js b/packages/editor/src/components/local-autosave-monitor/index.js index f0a4580e98f1d..e79fe0642b3af 100644 --- a/packages/editor/src/components/local-autosave-monitor/index.js +++ b/packages/editor/src/components/local-autosave-monitor/index.js @@ -188,7 +188,6 @@ function LocalAutosaveMonitor() { ); }