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.

Sign up for GitHub

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

Fixed-schedule AutosaveMonitor #23962

Merged
merged 16 commits into from
Jul 30, 2020
Merged

Fixed-schedule AutosaveMonitor #23962

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
be271b3
Initial stab at throttled, hook-based AutosaveMonitor
adamziel Jul 15, 2020
27315d7
Pass all tests
adamziel Jul 16, 2020
6bc161e
Simplify the logic and refactor use-scheduled-save to use-throttle
adamziel Jul 16, 2020
9d9d47a
Further simplify useThrottle
adamziel Jul 16, 2020
384d321
Further simplify useThrottle
adamziel Jul 16, 2020
5ea86aa
Simplify AutosaveMonitor
adamziel Jul 16, 2020
993077e
Fixed-schedule based AutosaveMonitor
adamziel Jul 16, 2020
5fc2211
Improve naming of things
adamziel Jul 16, 2020
b763c19
Remove obsolete shouldThrottle from AutosaveMonitor
adamziel Jul 16, 2020
c059ef3
Roll back obsolete changes
adamziel Jul 16, 2020
a54ccec
Revisit some of the unit tests for AutosaveMonitor
adamziel Jul 23, 2020
83e78bd
Adjust unit tests
adamziel Jul 28, 2020
dcc4549
Fix a few flaky autosave e2e tests
adamziel Jul 28, 2020
e117434
Fix a few flaky autosave e2e tests
adamziel Jul 28, 2020
c109577
Update sessionStorage key used in e2e tests
adamziel Jul 29, 2020
025de0f
Adjust the e2e test to throttle-less timer
adamziel Jul 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion packages/core-data/src/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
]
Comment on lines +663 to +667
Copy link
Contributor Author

@adamziel adamziel Jul 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this change the selector is not being updated in response to continued typing within the same paragraph block. This means the AutosaveMonitor is unaware of unsaved edits. It seems like a bug to me but I'm curious about your thoughts @youknowriad.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, it's a weird way to detect dirtiness though, I wonder if the component should rely on __experimentalGetDirtyEntityRecords instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a weird way. I suppose it was invented to account for the fact that autosaving doesn't make the post "not dirty" which also affects the isDirty prop this component receives. I just tested and the same is true for __experimentalGetDirtyEntityRecords - after an autosave completes, it still returns a non-empty list.

);
28 changes: 10 additions & 18 deletions packages/e2e-tests/specs/editor/various/autosave.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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' }`
);
}

Expand Down Expand Up @@ -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
Expand All @@ -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 () => {
Expand Down Expand Up @@ -219,7 +211,7 @@ describe( 'autosave', () => {
);
expect(
await page.evaluate( () => window.sessionStorage.length )
).toBe( 1 );
).toBeGreaterThanOrEqual( 1 );

// Trigger remote autosave
await page.evaluate( () =>
Expand Down Expand Up @@ -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 () => {
Expand All @@ -326,7 +318,7 @@ describe( 'autosave', () => {
);
expect(
await page.evaluate( () => window.sessionStorage.length )
).toBe( 1 );
).toBeGreaterThanOrEqual( 1 );

await page.reload();

Expand Down
74 changes: 32 additions & 42 deletions packages/editor/src/components/autosave-monitor/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this fixed 1000 number correct? Shouldn't this take "interval" into consideration?

Copy link
Contributor Author

@adamziel adamziel Aug 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be either 1000 or interval depending on how you look at it. Was there a bug report related to this?

A case for 1000:

  • We are not autosaveable and, from the perspective of this component, we don't know why – could be save in progress, could be internet connectivity issues
  • We want to know when we are saveable again as soon as possible because the interval could be 30 seconds and we could be intermittently non-autosaveable again on the next run. This would result in missing one or more autosave() calls.

A case for interval:

  • The handler explicitly asked the timer to run at most once per interval seconds
  • The handler is responsible for handling isAutosaveable property so it should already know if we missed an autosave() call

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, no bug, just looked weird. I was working on something unrelated requiring disabling autosave when I found it. Thanks for the explanation, it makes sense.

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() {
Expand All @@ -84,9 +74,9 @@ export default compose( [
const { interval = getEditorSettings().autosaveInterval } = ownProps;

return {
editsReference: getReferenceByDistinctEdits(),
isDirty: isEditedPostDirty(),
isAutosaveable: isEditedPostAutosaveable(),
editsReference: getReferenceByDistinctEdits(),
isAutosaving: isAutosavingPost(),
interval,
};
Expand Down
Loading