Autosave: fix conflict between remote and local autosaves #17501

mcsf · 2019-09-20T16:21:35Z

Fixes #17500

Description

For now, this only includes failing E2E tests

How has this been tested?

Screenshots

Types of changes

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.

gziolo · 2019-09-23T07:37:20Z

I can see it failing as expected but only on one of the nodes, it should fail for both suites of e2e tests (https://github.com/WordPress/gutenberg/pull/17501/checks?check_run_id=230152153):

mcsf · 2019-09-25T13:21:39Z

I can see it failing as expected but only on one of the nodes, it should fail for both suites of e2e tests (https://github.com/WordPress/gutenberg/pull/17501/checks?check_run_id=230152153):

Thanks for spotting. As of 208dbbb, this should be more reliable and only failing as intended.

gziolo · 2019-09-26T10:56:52Z

I don't see any changes in the logic. Should those e2e tests pass? :)

mcsf · 2019-09-26T11:03:15Z

I don't see any changes in the logic. Should those e2e tests pass? :)

No :(

packages/e2e-tests/specs/autosave.test.js

youknowriad · 2019-09-30T13:21:40Z

packages/editor/src/components/local-autosave-monitor/index.js

-function postKey( postId ) {
-	return `wp-autosave-block-editor-post-${ postId }`;
-}
-
 /**
 * Custom hook which returns a callback function to be invoked when a local
 * autosave should occur.
 *
 * @return {Function} Callback function.
 */
 function useAutosaveCallback() {


This refactoring is great, it also makes me think this particular hook is not necessarily needed anymore.

Perhaps, though we would still need to use requestIdleCallback somewhere. And wouldn't it be strange to have it in the action creator?

Or do you just mean inline the hook in the LocalAutosaveMonitor component?

Yeah, I meant inline, but I don't feel strongly tbh

packages/editor/src/components/local-autosave-monitor/index.js

youknowriad · 2019-09-30T13:24:13Z

packages/editor/src/components/local-autosave-monitor/index.js

-		if ( ! hasDifference ) {
-			// If there is no difference, it can be safely ejected from storage.
-			window.sessionStorage.removeItem( postKey( postId ) );
+		{


Is this needed? a way to create a scope?

Yes, it was for scope, and to share the WIP quickly

youknowriad · 2019-09-30T13:25:33Z

packages/editor/src/components/local-autosave-monitor/index.js

@@ -140,7 +143,7 @@ function useAutosaveNotice() {
 				},
 			],
 		} );
-	}, [ postId ] );
+	}, [ postId, remoteAutosave ] );


This means, the function is reexecuted each time removeAutosave changes which can happen on each autosave I guess and not only initial rendering, is this expected?

No, indeed, not expected. I think I left this here in my frustration with the E2E tests, when for some reason [ postId, hasFetchedAutosave ] didn't work. I can circle back.

youknowriad · 2019-09-30T13:27:20Z

packages/editor/src/store/controls.js

+
+export function localAutosaveClear( postId ) {
+	window.sessionStorage.removeItem( postKey( postId ) );
+}


What's the reasoning for keeping these utils here and using a control. I do wonder if we should just use these functions directly from like a autosave-utils or something like that?

I went for controls to see how it would feel to be able to dispatch saving actions just like with regular autosave.

autosave-utils

These would have to be available in E2E tests, so presumably they would be globally available. Is it worth adding a new module just for this?

I guess controls are flexible for both good and bad. What's your preference here?

I think since it has no relation with state, I'd personally prefer just a utility file somewhere

youknowriad · 2019-09-30T13:29:13Z

packages/editor/src/components/local-autosave-monitor/index.js

-			window.sessionStorage.removeItem( postKey( postId ) );
+		if (
+			( lastIsDirty.current && ! isDirty ) ||
+			( lastIsAutosaving.current && ! isAutosaving && ! didError )


Should this be the opposite (when the autosaves finishes instead of when the autosaves starts)?

Also, what's the reasoning for clearing when the post becomes dirty?

(when the autosaves finishes instead of when the autosaves starts)

Isn't that the case already? I guess useRef's notation with .current is confusing to read.

Also, what's the reasoning for clearing when the post becomes dirty?

Likewise, it clears when the post is flushed.

Also bump sleep time by 1s to allow time for local autosave.

toggleOfflineMode is a wrapper for Puppeteer's page.setOfflineMode which keeps track of the tester's intention to bring the network down. This allows the E2E framework to ignore logged network errors when these errors are expected.

* Switch to new sessionStorage handlers for local autosaves * Purge local autosave if remote autosave is successful * Add e2e test simulating unavailable network

* Defer to server autosave if any available * WIP commit. Clearly.

* hasFetchedAutosave should wait for user resolution * useAutosavePurge: only rely autosaving conclusion and lack of errors, don't look at isDirty anymore * Restore autosave() call in e2e test * Inline and remove useAutosaveCallback

* Until now, although we were requesting the entire autosave entity inside `LocalAutosaveMonitor`'s `useAutosaveNotice`, we only care about the existence or absence of an autosave for the current post. * `useAutosaveNotice` was requesting the entire entity because there were plans in #17501 to compare local and remote autosave objects, a requirement which was later dropped. * In contrast, the editor setting `autosave` is immediately available upon editor initialisation. * Later on, _if_ we choose to reconcile remote and local autosaves with more sophistication, this could possibly be done based on recency of autosave -- in which case we could might still get away with editor settings if the server provides this information inside the `autosave` object. * This change should hopefully provide a more reliable page-loading experience for autosave's E2E tests.

…and bailing on save failure (#17679) * Tests: Autosave: Allow extra reloading time for autosave notices * LocalAutosaveMonitor: Use immediately available autosave data * Until now, although we were requesting the entire autosave entity inside `LocalAutosaveMonitor`'s `useAutosaveNotice`, we only care about the existence or absence of an autosave for the current post. * `useAutosaveNotice` was requesting the entire entity because there were plans in #17501 to compare local and remote autosave objects, a requirement which was later dropped. * In contrast, the editor setting `autosave` is immediately available upon editor initialisation. * Later on, _if_ we choose to reconcile remote and local autosaves with more sophistication, this could possibly be done based on recency of autosave -- in which case we could might still get away with editor settings if the server provides this information inside the `autosave` object. * This change should hopefully provide a more reliable page-loading experience for autosave's E2E tests. * Tests: Keep retrying autosave * Tests: Autosave: Skip conflict-resolution test if unavailable remote autosave

mcsf added [Feature] Saving Related to saving functionality [Feature] Offline Related to offline editing labels Sep 20, 2019

mcsf requested review from ajitbohra, gziolo, nerrad, ntwb and talldan as code owners September 20, 2019 16:21

mcsf force-pushed the fix/autosave-conflict-remote-local branch from b3fd6c0 to 9e334a4 Compare September 25, 2019 10:22

mcsf mentioned this pull request Sep 25, 2019

Local autosave: Expand E2E tests #17503

Merged

youknowriad reviewed Sep 30, 2019

View reviewed changes

mcsf added 9 commits September 30, 2019 19:02

Add failing E2E tests

0dc8e87

Fix Promise.all idiom, make it actually parallel

6ff2ccd

Also bump sleep time by 1s to allow time for local autosave.

Don't parallelise, improve reliability

6e4c3c1

WIP: add e2e for clearing local autosave

7953a55

Testing: Utils: Add toggleOfflineMode

18273e1

toggleOfflineMode is a wrapper for Puppeteer's page.setOfflineMode which keeps track of the tester's intention to bring the network down. This allows the E2E framework to ignore logged network errors when these errors are expected.

Editor store: Controls: Add local autosave handlers

2fdfc1a

LocalAutosaveMonitor: Purge local only on successful remote autosave

4c8b4e8

* Switch to new sessionStorage handlers for local autosaves * Purge local autosave if remote autosave is successful * Add e2e test simulating unavailable network

WIP: Avoid server/local autosave conflicts

7ed5225

* Defer to server autosave if any available * WIP commit. Clearly.

Defer to remote autosaves. Address feedback.

072a828

* hasFetchedAutosave should wait for user resolution * useAutosavePurge: only rely autosaving conclusion and lack of errors, don't look at isDirty anymore * Restore autosave() call in e2e test * Inline and remove useAutosaveCallback

mcsf force-pushed the fix/autosave-conflict-remote-local branch from 040d0cb to 072a828 Compare September 30, 2019 18:02

Rename to __experimentalLocalAutosave

1b4e8ab

youknowriad approved these changes Sep 30, 2019

View reviewed changes

youknowriad merged commit 3e43bcc into master Sep 30, 2019

youknowriad deleted the fix/autosave-conflict-remote-local branch September 30, 2019 18:41

youknowriad pushed a commit that referenced this pull request Sep 30, 2019

Autosave: fix conflict between remote and local autosaves (#17501)

3ed9f49

youknowriad pushed a commit that referenced this pull request Sep 30, 2019

Autosave: fix conflict between remote and local autosaves (#17501)

432f341

mcsf mentioned this pull request Oct 1, 2019

Autosave: Improve E2E test reliability by consuming synchronous data and bailing on save failure #17679

Merged

5 tasks

mcsf mentioned this pull request Oct 21, 2019

Local autosave: Clear after successful save #18051

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Autosave: fix conflict between remote and local autosaves #17501

Autosave: fix conflict between remote and local autosaves #17501

mcsf commented Sep 20, 2019

gziolo commented Sep 23, 2019

mcsf commented Sep 25, 2019

gziolo commented Sep 26, 2019

mcsf commented Sep 26, 2019

youknowriad Sep 30, 2019

mcsf Sep 30, 2019

mcsf Sep 30, 2019

youknowriad Sep 30, 2019

youknowriad Sep 30, 2019

mcsf Sep 30, 2019

youknowriad Sep 30, 2019

mcsf Sep 30, 2019

youknowriad Sep 30, 2019

mcsf Sep 30, 2019

youknowriad Sep 30, 2019

youknowriad Sep 30, 2019

mcsf Sep 30, 2019

Autosave: fix conflict between remote and local autosaves #17501

Autosave: fix conflict between remote and local autosaves #17501

Conversation

mcsf commented Sep 20, 2019

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

gziolo commented Sep 23, 2019

mcsf commented Sep 25, 2019

gziolo commented Sep 26, 2019

mcsf commented Sep 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment