-
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
Remove autosaveable check from autosave monitor #10431
Conversation
bf8d656
to
8ebdf97
Compare
As I understand it, the issue may surface in that the endpoint returns error responses when a save is not needed. I've suggested before we handle this more gracefully, perhaps from the server, or ignoring the error response from the client. |
We could potentially respect the current behavior by moving the |
I've been thinking more on this. A few ideas come to mind:
The last of these seems like the most direct solution for the near-term, while allowing for future refactorings to improve upon its wastefulness. There are additional options which surface if we'd have better insight into specific changed properties (#7409) since currently our dirty detection is rather "dumb". |
I'll defer on the code review, as I don't have a strong understanding of the current abstraction. |
@danielbachhuber FYI you were tagged for review since you were the sole approving reviewer / merger of #6257 wherein the issue was introduced. |
Related alternative approach, which is a more involved refactoring: #10844 The viability of this one hasn't yet been determined. |
That seems like a reasonable approach and should be the only time we really need to check this. Did you attempt that in this PR? |
@@ -157,7 +158,7 @@ export default compose( [ | |||
isDirty: isEditedPostDirty(), | |||
isNew: isEditedPostNew(), | |||
isSaveable: isEditedPostSaveable(), | |||
isAutosaveable: isEditedPostAutosaveable(), | |||
isEditedPostAutosaveable: isEditedPostAutosaveable, |
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 is confusing, why does it pass down selector rather than calling it 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.
This is confusing, why does it pass down selector rather than calling it here?
From original comment:
I'm also not sure if it would be a good idea to pass the selector function as a prop instead of the value, but like that we can call it only when needed.
It is a bit strange, though I understand why it's being done. I think though that if we were to go this direction, I'd rather we move toward dropping all consideration of whether the post is autosaveable, and just use the "is dirty" with error tolerance from the autosave REST response for unneeded saves.
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.
Either way, we should document the solution to ensure that it doesn't get refactor by accident.
I'm going to move this out of 4.2 for now. but if you think it's ready please bring it back. |
I don't think this is a viable approach due to how we re-"dirty" state after a save completes if there are remaining unsaved edits, as it has the very likely potential to become stuck in an infinitely repeating sequence of delayed autosaves. This occurs because we only pick gutenberg/packages/editor/src/store/effects/posts.js Lines 179 to 186 in 9d46596
|
Superseded by #10844 |
Description
This would be a quick fix for #10427. I'm not sure on the implications of removing the check in the autosave monitor. I'm also not sure if it would be a good idea to pass the selector function as a prop instead of the value, but like that we can call it only when needed. Again, unsure here if could get rid of this check and selector altogether... Would like some feedback around these issues from people who have worked on autosave before like @adamsilverstein and @aduth.
How has this been tested?
Ensure serialisation does not run on every keystroke after the initial autosave. This could be done by logging it.
Screenshots
Types of changes
Checklist: