Migrate editor store to thunks #35929
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: -32 B (0%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
|
Looks good from eyeballing and the e2e tests pass. Great work @jsnajdr! React native failure looks suspicious so I restarted it. Even if it's a legit failure, it should be simple to fix. Would you like any help with migrating unit tests? |
5 tasks
This was referenced Feb 7, 2022
jsnajdr
force-pushed
the
thunkify/editor-store
branch
from
e9f0118 to
e4b394c
Compare
jsnajdr
force-pushed
the
thunkify/editor-store
branch
from
e4b394c to
e309ad8
Compare
|
I updated (or rewrote 🙂) the unit tests and this migration is now ready for testing, review and shipping. The original tests for actions, especially for the There are two related bugfixes in this PR that are not strictly
|
jsnajdr
force-pushed
the
thunkify/editor-store
branch
from
e309ad8 to
99781b8
Compare
adamziel
reviewed
Feb 14, 2022
| @@ -466,7 +466,7 @@ export const saveEntityRecord = ( | |||
| // so the client just sends and receives objects. | |||
| const currentUser = select.getCurrentUser(); | |||
| const currentUserId = currentUser ? currentUser.id : undefined; | |||
| const autosavePost = resolveSelect.getAutosave( | |||
| const autosavePost = await resolveSelect.getAutosave( | |||
There was a problem hiding this comment.
How did we miss it? :o great catch!
There was a problem hiding this comment.
The situation we'd need looks like: the post has edits for content, a previous autosave for title, and now we want to do a new autosave. And we want to autosave the full { content, title } tuple because both attributes are changed. But under what circumstances is it possible that the title changes are in autosave but not in local edits?
@jsnajdr Hm perhaps when if the autosave was created in one editing session, then the page was refreshed, and the new editor uses that autosave but without any local edits? On the other hand, if the await was missing for a few months and noone noticed any difference, maybe we could remove that entirely 🤔 let's get this PR in, research any familiar-looking issues, and try removing that in a follow-up PR
There was a problem hiding this comment.
Hm perhaps when if the autosave was created in one editing session, then the page was refreshed, and the new editor uses that autosave but without any local edits?
In that case, when I refresh the page, the editor tells me that there is an existing autosave and asks if I want to apply it? If I don't, it's discarded, if I do, the autosave changes become edits?
if the
awaitwas missing for a few months and noone noticed any difference, maybe we could remove that entirely
It's also possible that people were actually losing data in a rare edge case, without ever having enough info to report an issue, and that's not a good thing. We should certainly investigate this further.
adamziel
reviewed
Feb 14, 2022
| ); | ||
| } | ||
| */ | ||
| export const editPost = ( edits, options ) => ( { select, registry } ) => { |
There was a problem hiding this comment.
this reads so much better now
adamziel
reviewed
Feb 14, 2022
| * | ||
| * @return {Object} An action object | ||
| */ | ||
| export function __experimentalRequestPostUpdateFinish( options = {} ) { |
There was a problem hiding this comment.
It's __experimental so removing should be fine, I didn't find any remaining usages.
adamziel
reviewed
Feb 14, 2022
| @@ -401,10 +307,9 @@ export function createUndoLevel() { | |||
| } | |||
|
|
|||
| /** | |||
| * Returns an action object used to lock the editor. | |||
| * Action that locks the editor. | |||
There was a problem hiding this comment.
How about just
Suggested change
| * Action that locks the editor. | |
| * Locks the editor. |
without "Action that"?
There was a problem hiding this comment.
We use "Action that ..." quite universally when documenting actions, there are almost 100 usages of that. Therefore I'd rather keep it as is.
adamziel
reviewed
Feb 14, 2022
| */ | ||
| export function* autosave() { | ||
| export const autosave = () => () => { |
There was a problem hiding this comment.
Is this covered by React Native tests?
There was a problem hiding this comment.
I don't know, I didn't check that. But the change we're doing here looks trivial.
There was a problem hiding this comment.
It does, I was just worrying whether thunks work properly in RN.
adamziel
approved these changes
Feb 14, 2022
There was a problem hiding this comment.
Great work! The actions read so much cleaner, the comments do much better job explaining the code, and the tests are more solid this way. On the first sight it may seem like we're testing less, but I believe the opposite is true – this checks the overall store actual behavior vs confirming that a function returns what it says.
Thank you for tackling this one! I left a few comments, but I don't think there's anything blocking (unless we have no RN coverage on the autosave action).
Yeah I totally agree. When writing the new unit tests, I even discovered a bug -- the one with incorrect notice after trashing. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Draft PR for the purpose of e2e running and @adamziel reviewing 🙂