[Edit Post]: Migrate store actions to thunks #36551
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: +28 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
ntsekouras
force-pushed
the
add/edit-post-thunks
branch
2 times, most recently
from
6bb92fc to
590f5e6
Compare
ntsekouras
commented
Dec 13, 2021
adamziel
reviewed
Dec 15, 2021
Comment on lines
+182
to
+184
| registry | ||
| .dispatch( interfaceStore ) | ||
| [ isPinned ? 'unpinItem' : 'pinItem' ]( 'core/edit-post', pluginName ); |
There was a problem hiding this comment.
I had to stop here for a moment to consider all the logic. Perhaps it would read smoother without the ternary operator?
Suggested change
| registry | |
| .dispatch( interfaceStore ) | |
| [ isPinned ? 'unpinItem' : 'pinItem' ]( 'core/edit-post', pluginName ); | |
| const { unpinItem, pinItem} = registry.dispatch( interfaceStore ); | |
| if ( isPinned ) { | |
| unpinItem( 'core/edit-post', pluginName ); | |
| } else { | |
| pinItem( 'core/edit-post', pluginName ); | |
| } |
There was a problem hiding this comment.
Ideally (out of scope for this PR) there would be a togglePinnedItem action in the interface store. The pinItem/unpinItem actions are used at two places, and both of them really want to toggle.
adamziel
reviewed
Dec 15, 2021
adamziel
reviewed
Dec 15, 2021
adamziel
reviewed
Dec 15, 2021
adamziel
reviewed
Dec 15, 2021
@ntsekouras There was a similar problem in gutenberg/packages/edit-navigation/src/store/test/resolvers.js Lines 40 to 45 in 8b1d86e It still required mocking the API response though: Would that make it easier here? If not, I think leaning on e2e is reasonable. |
ntsekouras
force-pushed
the
add/edit-post-thunks
branch
from
2c89e7f to
74bcc65
Compare
jsnajdr
reviewed
Feb 14, 2022
Comment on lines
+182
to
+184
| registry | ||
| .dispatch( interfaceStore ) | ||
| [ isPinned ? 'unpinItem' : 'pinItem' ]( 'core/edit-post', pluginName ); |
There was a problem hiding this comment.
Ideally (out of scope for this PR) there would be a togglePinnedItem action in the interface store. The pinItem/unpinItem actions are used at two places, and both of them really want to toggle.
|
|
||
| if ( ! isEditorReady ) { | ||
| return; | ||
| } | ||
|
|
||
| const postType = yield controls.select( editorStore, 'getCurrentPostType' ); | ||
|
|
||
| // Only initialize once. | ||
| if ( metaBoxesInitialized ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Interestingly, the "is editor ready" and "are metaboxes initialized" checks are already performed by the <MetaBoxes> component before calling initializeMetaBoxes. One of the checks is redundant.
| subscribe( () => { | ||
| const isSavingPost = select( editorStore ).isSavingPost(); | ||
| const isAutosavingPost = select( editorStore ).isAutosavingPost(); | ||
| registry.subscribe( async () => { |
There was a problem hiding this comment.
This subscription doesn't really belong to a Redux action. It's something that would be much more easily implemented in the <MetaBoxes> component. The component would then (elegantly) do three things:
function MetaBoxes() {
// initialize metaboxes
useEffect( () => {
if ( shouldInit ) { initializeMetaBoxes(); }
}, [ shouldInit ] );
// update metaboxes (new)
useEffect( () => {
if ( shouldUpdate ) { requestMetaBoxUpdates(); }
}, [ shouldUpdate ] );
// render metaboxes
return <>{ metaBoxes.map( box => ... ) }</>;
}| } catch { | ||
| yield controls.dispatch( editPostStore, 'metaBoxUpdatesFailure' ); | ||
| dispatch.metaBoxUpdatesFailure(); |
There was a problem hiding this comment.
Interesting that both these actions do the same thing: update the isSavingMetaBoxes flag. They could be merged into one. We are ignoring the errors.
Also, do they need to be a part of the public API? There is no use case for that. Just like REQUEST_META_BOX_UPDATES, they could be internal/private.
| registry | ||
| .select( interfaceStore ) | ||
| .isFeatureActive( editPostStore.name, 'welcomeGuide' ) | ||
| ).toBeFalsy(); |
There was a problem hiding this comment.
isFeatureActive always returns a true boolean, not just a falsy value. A stronger .toBe( false ) expectation is in place.
jsnajdr
force-pushed
the
add/edit-post-thunks
branch
from
74bcc65 to
37abf43
Compare
jsnajdr
approved these changes
Feb 14, 2022
There was a problem hiding this comment.
Looks good. I posted a few comments, but they are more like refactoring ideas rather than blockers.
There was an incorrect update of package-lock.json and I added a commit that fixes that.
I also rebased the PR onto the latest trunk. Let's see what the CI checks say now.
|
The e2es are failing at: The PR is changing exactly this metabox-saving code, so apparently at some point the migration went wrong. |
jsnajdr
force-pushed
the
add/edit-post-thunks
branch
from
eccc9c0 to
ba9d323
Compare
|
Thanks for the reviews here! Also thank you @jsnajdr for the extra work with rebasing and fixes. I'm now looking at the comments and will try to find out about the metaboxes test failures. As I'm not really familiar with metaboxes, I could use some help here if you find something obvious, but either way I'll try to solve them. I can see that some comments about refactoring some things in metaboxes make sense, but I'd love if we could just refactor to use thunks here and handle these in a follow up. |
@ntsekouras I did some debugging and it seems that the e2e test is inherently fragile and flaky. Here's approximately what it does: await savePost();
reloadPage();
verifySavedInfoHasBeenLoaded();I.e., it reloads the page immediately after subscribe( async () => {
if ( prevIsSaving === true && nextIsSaving === false ) {
await dispatch( requestMetaBoxUpdates() );
}
} );Is the metabox save request performed before the |
|
Thanks for helping @jsnajdr!! I'm still looking into the failures and it might be something related with the |
I also tried the patch with the |
jsnajdr
reviewed
Feb 15, 2022
|
The e2e tests are now green after the |
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.
This PR migrates the
edit-postdata store to thunks and updates unit tests.Notes
There are some tests missing right now, but when I started writing them a lot of mocking for
corestore internals were needed and I'm not sure if it's the right approach here. Maybe some e2e tests would be better for these cases?Namely the untested actions are
__unstableCreateTemplate, initializeMetaBoxes and requestMetaBoxUpdates.