Fixed-schedule AutosaveMonitor #23962
Conversation
adamziel
added
[Type] Bug
| prevProps.editsReference !== editsReference | ||
| isDirty && | ||
| isAutosaveable && | ||
| ! didAutosaveForEditsReference.current |
There was a problem hiding this comment.
It's very unclear to me why this check is necessary? Why not just always "schedule an autosave on each edit change"?
There was a problem hiding this comment.
I guess the reason is because you have "cancelSave" as an API to call explicitely while IMO, it should be handled internally in useScheduleSave. Basically if there's a new scheduled autosave while one is awaiting to be triggered, cancel it and schedule the new one instead.
There was a problem hiding this comment.
I like the new approach.
There was a problem hiding this comment.
Basically if there's a new scheduled autosave while one is awaiting to be triggered, cancel it and schedule the new one instead.
This would be the same as the current debounced approach which causes long periods without an autosave as long as the user is typing. As discussed in the issue, we don't need to limit the amount of these requests as the autosave would amend the last autosaved revision instead of creating a new one every time.
I want to make it so once an autosave is scheduled, no subsequent change would cancel it. It simply stays scheduled at a fixed point in time, and can only be cancelled if the AutosaveMonitor is told it shouldn't act (e.g. isAutosaveable == false).
I am struggling with the number of corner cases to handle though and have an idea for a much simpler and more bullet-proof approach - I will update this PR soon to show exactly what I mean.
|
Size Change: -16 B (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
| ( state ) => [ | ||
| state.undo.length, | ||
| state.undo.offset, | ||
| state.undo.flattenedUndo, | ||
| ] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good catch, it's a weird way to detect dirtiness though, I wonder if the component should rely on __experimentalGetDirtyEntityRecords instead.
There was a problem hiding this comment.
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.
|
@youknowriad @mcsf Would you mind taking a look at this PR and sharing your thoughts? Ignore failing tests - I'm stalling until we agree on the approach here. |
adamziel
changed the title
|
I like this approach, it works as intended for me. it might be good to document the behavior somewhere. Maybe a comment on the component. |
|
Sorry for dropping the ball and not helping here, guys. |
adamziel
force-pushed
the
update/make-autosave-timer-more-reliable
branch
from
799bb38 to
c109577
Compare
| delete this.pendingSave; | ||
| autosaveTimerHandler() { | ||
| if ( ! this.props.isAutosaveable ) { | ||
| this.setAutosaveTimer( 1000 ); |
There was a problem hiding this comment.
Is this fixed 1000 number correct? Shouldn't this take "interval" into consideration?
There was a problem hiding this comment.
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
intervalcould be 30 seconds and we could be intermittently non-autosaveable again on the next run. This would result in missing one or moreautosave()calls.
A case for interval:
- The handler explicitly asked the timer to run at most once per
intervalseconds - The handler is responsible for handling
isAutosaveableproperty so it should already know if we missed anautosave()call
There was a problem hiding this comment.
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.
Description
The current AutosaveMonitor does not consistently execute autosaves in response to user input.
This PR proposes a completely different approach. Instead of trying to get scheduling and cancelling right, we:
needsAutosaveand flip it to true or false anytime something relevant happens.isAutosaveableis false, keep retrying every 1 second.As a result, autosave will be executed at most 60 seconds after the first unsaved input, sometimes less.
How has this been tested?
Unit tests are failing now - I'd like to have some discussion about the approach before spending time on adjusting them.
For manual testing, see the instructions below:
Default autosave interval is 60 seconds, if you want to save some time you could modify line 81 to say
interval: 4,instead of justinterval,. Then:Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist: