-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Editor: Add sessionStorage autosave mechanism #16490
Conversation
Sounds like a good first step. I'd love to see using service workers for this though, as it makes for a more robust experience when offline. |
@swissspidy Do you see service workers as an alternative to this approach, or a supplement? It's been some time since I've worked with service workers, so any resources you can share would be appreciated. |
Using serve workers would be a supplement, especially because they require HTTPS. In #6322 @westonruter actually shared some very good ideas on how they could enhance the editing experience when offline. For example, it can cache assets and even REST API responses locally. So when you are offline, you could still refresh the page, select tags, and write content as usual. You could even go and edit some totally different post and have all the (cached) data at your fingertips. And thanks to background sync, any attempt to save the post will be recorded and run as soon as you get online again. While https://developer.mozilla.org/en-US/docs/Web/API/Service_Worker_API covers the fundamental basics of service workers, I recommend checking out the PWA feature plugin as it includes some of these things I mentioned already, just for other parts of WordPress. For example, it has background sync for comments and support for a dedicated offline template that is shown when there's no cached content. |
Thanks @swissspidy . These all sound like worthwhile explorations. In the short term, I think we can still pursue the approach in this pull request independently, and keep #6322 open for further discussion and enhancements. |
I've finished fleshing out the remaining functionality, and have updated the original comment with an animation, implementation notes, and testing instructions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to be on the right track. 👍
autosave: dispatch( 'core/editor' ).autosave, | ||
withDispatch( ( dispatch, ownProps ) => ( { | ||
autosave() { | ||
const { autosave = dispatch( 'core/editor' ).autosave } = ownProps; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An option might be to extract a more generic BaseAutosaveMonitor
component.
From an interface point of view, it feels a bit unusual to have this fall back to performing a regular autosave if for some reason the autosave
prop is undefined. Especially given the current usage where there are multiple of these registered:
<AutosaveMonitor />
<LocalAutosaveMonitor />
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An option might be to extract a more generic
BaseAutosaveMonitor
component.
Hmm, I see where you're coming from. Some of this is inherited from the previous behavior that the editor has some understanding of what an "autosave" action means (the server-persisted autosave).
Funny though that your recommendation is essentially the opposite of @mtias at #16490 (comment) 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😄 Hah, true.
Combining them would allow more coordination between the two autosave systems, which might be beneficial.
} ) ); | ||
|
||
return useCallback( () => { | ||
window.localStorage.setItem( 'post_' + postId, JSON.stringify( { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The REST autosave system has some checks in place to make sure either post_title or content is defined before saving. Worth replicating that here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also should use sessionStorage
. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, let's switch to sessionStorage
-
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The REST autosave system has some checks in place to make sure either post_title or content is defined before saving. Worth replicating that here?
Could you point me to what it is you're referring to on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, it's isEditedPostSaveable
:
https://github.com/WordPress/gutenberg/blob/master/packages/editor/src/store/selectors.js#L458-L462
Which is used to return early from savePost
in the event there aren't changes to any of those autosave fields:
https://github.com/WordPress/gutenberg/blob/master/packages/editor/src/store/actions.js#L439
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expect the overhead for saving to sessionStorage is very low, possibly lower than retrieving and comparing to the old data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expect the overhead for saving to sessionStorage is very low, possibly lower than retrieving and comparing to the old data?
Agreed, unless we measure otherwise.
didSave: select( 'core/editor' ).didPostSaveRequestSucceed(), | ||
} ) ); | ||
|
||
const lastDidSave = useRef( didSave ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works really nicely in testing. I was curious about when didSave
would get set to false again after a successful save. It turns out that it's set to false when the next save request is initiated (possibly a bit confusing since it's not yet an unsuccessful save, which false might indicate to some).
It might be worth adding a brief comment that describes this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. Now that this PR had been rebased on top of the merged core entities changes (#17368), useAutosavePurge
no longer works, as didSave
is defaulting to true.
Neither scenario is wrong, and both are unspecified behaviour. :)
Instead, this hook will need to observe some additional state, perhaps publish/draft status, or just dirtiness. @epiqueras, considering how entities now work, does any of these sound better to explore than the other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is what you want?
const { isSavingEntityRecord, getLastEntitySaveError } = select('core/data')
const didSave =
isSavingEntityRecord(...args) === false && !getLastEntitySaveError(...args)
Note that isSavingEntityRecord(...args)
is undefined before the first save, hence the === false
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had pushed something that seems to be working that's based on isDirty
instead. If it won't cover our needs we could look at this.
actions: [ | ||
{ | ||
label: __( 'Restore the backup' ), | ||
onClick() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hard part in terms of the user experience here is that this action might destroy more of the user's content if the user has added more to the post since the last local autosave. Although they can use undo, I don't think that's always apparent.
The other autosave system displays a diff before giving the option to apply changes. Although it's a bit of extra work, it could be worth trying. I believe there's a diff library already in use for block validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hard part in terms of the user experience here is that this action might destroy more of the user's content if the user has added more to the post since the last local autosave. Although they can use undo, I don't think that's always apparent.
The other autosave system displays a diff before giving the option to apply changes. Although it's a bit of extra work, it could be worth trying. I believe there's a diff library already in use for block validation.
By other autosave, I assume you mean the revisions? There was also a localStorage/sessionStorage-based solution in the classic editor which, as far as I recall, took no such precautions.
I'm leaning more on the end that Undo should serve as the intended option should the changes not be what the user was expecting, maybe at least as a first pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I mean revisions. Wouldn't want to add too much more to this PR, so if considering that part separately is an option, that sounds good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, in the classic editor we show the user the following message when an autosave is present in sessionStorage:
Can we match something like that messaging here? Is this still true in Gutenberg, can the user use 'undo' to undo the restore?
The other autosave system displays a diff before giving the option to apply changes. Although it's a bit of extra work, it could be worth trying. I believe there's a diff library already in use for block validation.
Great idea! I suggest we revisit this when we develop a more full fledged revision UI into Gutenberg.
|
||
return ( | ||
<AutosaveMonitor | ||
interval={ AUTOSAVE_INTERVAL_SECONDS } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to require that the user is idle for 15 seconds before it'll trigger a save.
Since this is a last resort, I wonder if there's also an option to use additional strategies for saving local autosaves, like the beforeUnload handler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that the initial feel is that this isn't eager enough to locally autosave.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that localStorage
is a synchronous API, should requestIdleCallback
be used if available?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps related: https://github.com/WordPress/gutenberg/pull/16490/files#r303243055
Maybe we should only save locally if you cannot save otherwise? Would it make more sense then to implement it in a different layer of the application?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a last resort, I wonder if there's also an option to use additional strategies for saving local autosaves, like the beforeUnload handler.
I'd agree on this, and the fact that we should probably just schedule (or at least check for changes to justify) the autosaves to occur every 15 seconds, not just 15 seconds after the last change.
Given that
localStorage
is a synchronous API, shouldrequestIdleCallback
be used if available?
It could be worth exploring. I'd question whether it would make a measurable difference, however (related: #16657).
|
||
if ( ! hasDifference ) { | ||
// If there is no difference, it can be safely ejected from storage. | ||
window.localStorage.removeItem( 'post_' + postId ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This key format would mean that if a second user logs in to the same browser, they can restore an autosave made by the first user.
Not sure if that's an issue, but I know the current autosave system stores revisions by different users separately. One way to align with that but still keep the same localstorage key is to save the userId in the payload and check the value before showing the notice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This key format would mean that if a second user logs in to the same browser, they can restore an autosave made by the first user.
Based on other comments, I think sessionStorage
may be enough to resolve this.
Though technically it could still be an issue if the user logs out of the current user and into another user while still within the same tab / session. I'd be curious to know if / how this was addressed in the classic editor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently in core we clear autosave data from session storage when the user logs out: https://github.com/WordPress/wordpress-develop/blob/6dad32d2aed47e6c0cf2aee8410645f6d7aba6bd/src/wp-login.php#L103 as long as we use the same key format, this code should continue to work.
|
||
const noticeId = uniqueId( 'wpEditorAutosaveRestore' ); | ||
|
||
createWarningNotice( __( 'The backup of this post in your browser is different from the version below.' ), { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just thinking of all the weird and wonderful edge cases that might happen. I think there is a chance that a user might create local autosave on computer A, a revision for a post on a computer B, and then when returning to the computer A they'd be presented with both this notice and the normal autosave notice.
One option would be to timestamp local saves, and then only present the user with the notice for the most recent save whether it's a local or a server stored save.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Timestamping could help, but I'm not sure about any edge cases that may miss.
There's a few ways in which we may lead the user to make mistakes. One way to mitigate it is to tie this with the diffing view we use in block validation (which incidentally is also in need of love). edit: I now see that another comment by Dan suggests the same
|
||
const noticeId = uniqueId( 'wpEditorAutosaveRestore' ); | ||
|
||
createWarningNotice( __( 'The backup of this post in your browser is different from the version below.' ), { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Timestamping could help, but I'm not sure about any edge cases that may miss.
There's a few ways in which we may lead the user to make mistakes. One way to mitigate it is to tie this with the diffing view we use in block validation (which incidentally is also in need of love). edit: I now see that another comment by Dan suggests the same
{ | ||
label: __( 'Restore the backup' ), | ||
onClick() { | ||
editPost( omit( edits, [ 'content' ] ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per our conversation:
Let's see if we really need to pull 'content'
out of edits
for this particular editPost
action. resetEditorBlocks
should be taking care of the cleanup of edits.content
in state. I'd rather have this use of editPost
be immediately clear, even if it's unnecessarily setting edits.content
.
|
||
return ( | ||
<AutosaveMonitor | ||
interval={ AUTOSAVE_INTERVAL_SECONDS } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that the initial feel is that this isn't eager enough to locally autosave.
This probably won't matter so much, but I wanted to mention: Also, See also |
WordPress Core currently uses |
Thanks all for the feedback! A few specific action items I've observed:
|
On occasion I have some hickups - as in a slow internet causing this: (I should perhaps instead say that the web sites have some hickups..:) #17015 It would be very helpful to have this PR merged! Thanks. |
As Andrew is enjoying some time off, I've revived this branch and applied some of the feedback that's been provided. A couple of notes:
I don't think this is something to address in this PR, but it's something to be aware of. |
* Base implementation on undo state of core entities * Expand unit tests
ef43fcd
to
c6c7a8e
Compare
8e1c26f
to
52ca63e
Compare
* | ||
* @return {*} A value whose reference will change only when an edit occurs. | ||
*/ | ||
export const getReferenceByDistinctEdits = createSelector( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to provide backward compatibility support here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// The `shouldThrottle` props allows overriding this behaviour, thus | ||
// making the autosave action "throttled". | ||
if ( ! shouldThrottle ) { | ||
clearTimeout( this.pendingSave ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should clearTimeout
call be guarded here to avoid executing this block when this.pendingSave
is not defined?
}, [ isDirty ] ); | ||
} | ||
|
||
function LocalAutosaveMonitor() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to add some docs as a follow-up PR since it's a public API :)
return useCallback( () => { | ||
const saveToSessionStorage = () => { | ||
window.sessionStorage.setItem( postKey( postId ), JSON.stringify( { | ||
post_title: getEditedPostAttribute( 'title' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about other stuff like:
- taxonomy
- categories
- featured image
and so on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth noting that this wasn't saved in the classic editor, but that doesn't mean we should do the same. Just noting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. Grzegorz and I also talked a little about this:
-
There's also a management of expectations to play with: the more data we autosave, the greater the expectations that this is a fully fledged offline support feature, which it isn't.
-
I think backing up post content really is 90% of the feature, and for an introductory PR this is more than fine. The fact that Classic did the same validates this.
-
As Gutenberg evolves to support more than just posts with content and into the realm of full-site editing, a lot of this might become a lot more ambitious if we're dealing with multiple entities to save in a single editor. (When we get there we can try autosaving around each
BlockEditor
and see where it takes us.)
For now, let's keep it focused on these three bits.
} ) ); | ||
}; | ||
requestIdleCallback( saveToSessionStorage ); | ||
}, [ postId, getEditedPostAttribute ] ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getEditedPostAttribute
- we put this action as a dependency here, but we don't do it for all actions in useAutosaveNotice
. The question is why it is necessary here or why do we miss all those actions for useEffect
inside useAutosaveNotice
.
Is it because of useAutosaveNotice
should be triggered only once per post id?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per individual chat, let's drop getEditedPostAttribute
from the deps. We don't yet have strong conventions on how to deal with this in React hooks.
If we were very strict about this we would list every single reference used in the closure, but the references in our case are all static constant functions, either directly (defined in the module or imported) or indirectly (selectors obtained via select
). I don't feel strongly, but I'd err toward keeping references out for now and adding them if we tighten our expectations here.
pressKeyWithModifier, | ||
} from '@wordpress/e2e-test-utils'; | ||
|
||
const AUTOSAVE_INTERVAL_SECONDS = 15; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a big number of seconds to wait in the test, to be fair. Can we set something lower through some sort of override? In other words, can we avoid using sleep as much as it is possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could bring it down to 5, if that makes sense as a user experience.
It might also be beneficial to make this an editor setting so it could be customised. Would this be a requirement for this particular PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How slow would setting session storage be for large posts?
It needs to be profiled, and we also need to benchmark with the perf e2e suite.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works great, let's move forward with it once we add back deprecated message for the removed selector with the proper warning printed to the console. See: #16490 (comment)
As a follow-up tasks we should do the following:
- Add documentation for the newly introduced component
- Make the local autosave interval configurable so sites could tweak it and at the same time to speed up e2e tests
- Let's add more e2e tests which cover other use cases like (mostly to prevent regressions):
- logging out - which wipes the saved post
- ensuring that the saved content doesn't get loaded in the other post
A performance benchmark: master
local autosave, no editor setting
local autosave, with editor setting
|
beforeEach( async () => { | ||
await clearSessionStorage(); | ||
await createNewPost(); | ||
await setLocalAutosaveInterval( AUTOSAVE_INTERVAL_SECONDS ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be an associated tear down to this, to avoid the state bleeding over into other tests run in the same container?
Not sure it would have any noticeable impact since ideally those tests would be resilient regardless of an autosave interval, but seems like an undesirable side-effect in any case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it might depend if setLocalAutosaveInterval
is something which would persist outside the current page. If most tests would create a new post and thus navigate away, it might not be a problem.
Then again, it's long been a desire of mine that createNewPost
shouldn't have to trigger a full page reload (see #18668).
Closes #7367
Related: #6322
Some edits by @mcsf
This pull request seeks to implement periodic
localStoragesessionStorage autosave backups for posts. It reuses and extends the behavior of the existing server-persisted autosave component, leveraging sessionStorage when available to save data on a more frequent basis.Implementation notes:
This was designed to be compatible with localStorage backup mechanism implemented in the classic editor (WordPress 4.9.x and older), hence the selection of the localStorage key'post_' + postId
.This influenced the choice to not use the existing data persistence behavior, along with a desire to avoid creating saves until the interval has elapsed (vs. on every state change).
Even with the delay, we may want to explore using
requestIdleCallback
in addition to the timeout interval, since serialization can be a very non-performant operation, depending on the size of the post.Testing Instructions:
The autosaves occur every 15 seconds after the last change has occurred. Verify that upon waiting this duration and reloading the page (dismissing the prompt), you see a restore option to restore the title, content, and excerpt of the post you had been working on.
Verify that the prompt is not shown if you proceed to make edits to the post and save (regardless of whether you choose to restore or dismiss the notice).
Checklist: (Added 2019-08-30)
Still try to save to storage only if a change has actually occurredShould confirm that this data is in-fact purged upon logging out, or that an equivalent measure is taken to avoid access by other users even within the same storageuseAutosavePurge
, per Editor: Add sessionStorage autosave mechanism #16490 (comment)