PostPreviewButton: rewrite to functional, avoid state transitions in lifecycles #44971
Conversation
|
Size Change: -241 B (0%) Total Size: 1.43 MB
ℹ️ View Unchanged
|
Do you think we can add an unstable action (action in the sense of |
Yes, I will work on that 👍 It's quite a detour from the original React 18 migration, but I believe it will improve the post save logic a lot. We'll probably also need filters for the "is autosaveable?" and "is dirty?" values. Now it's implemented with special like |
Yeah I guess, even if I don't like it much 😬 (a filter in a selector might not be the greatest idea as it doesn't retrigger the subscription on change). My ultimate goal is to actually remove the "editor" package entirely as now, it just became a subset of edit-post and this would remove these hacks but let's leave that for a far future. |
jsnajdr
force-pushed
the
update/post-preview-button
branch
from
ff23f1e
to
6071a5a
Compare
|
Hi @youknowriad 👋 I implemented the I was able to remove all the Removing the remaining I'm not sure how to name the By the way, to test the metaboxes I installed the ACF plugin, and noticed they override the
That would be a good use case for an |
jsnajdr
force-pushed
the
update/post-preview-button
branch
from
6071a5a
to
1b8dcb3
Compare
Hi @lgladdy 👋 It's great that we connected so quickly 🙂 Some catch handler will still need to be there, but placed slightly differently: editor.savePost = function() {
return acf.validateFormPromise().then(
() => {
return originalSavePost();
},
( err ) => {
// ignore validation error
}
);
}This code has the following behavior:
|
jsnajdr
force-pushed
the
update/post-preview-button
branch
from
1b8dcb3
to
a433283
Compare
|
I fixed some unit tests:
Everything is green now and ready for final review. The main thing we need to decide on is the exact name of the hook and its parameters. |
| .select( coreStore ) | ||
| .getLastEntitySaveError( | ||
| 'postType', | ||
| previousRecord.type, | ||
| previousRecord.id | ||
| ); | ||
|
|
||
| if ( ! error ) { | ||
| await applyFilters( |
There was a problem hiding this comment.
This looks more like an "action" than a filter, the only reason it's a filter is because we want it to be async. This makes me wonder if we shouldn't just support "async actions" instead.
await doAction( 'editor.SavePost' )
There was a problem hiding this comment.
Yes, webpack also distinguishes between sync and async hooks, and it's a good thing to have in a JavaScript project.
We could introduce a new doAsyncAction API which is similar to doAction but awaits between calls and aborts on a rejected promise. The addAction API could stay the same. The entire difference is in how actions are called, not how they are registered.
If the caller makes a mistake and calls doAction instead of doAsyncAction, we could issue a warning in case an action callback returns a promise (or thenable in general) that would be ignored by doAction.
There was a problem hiding this comment.
curious why this needs to be async though, is the thought that options might get changed? if so pass that to the filter, if not just use action without async?
There was a problem hiding this comment.
It's async because saving post is in principle async. It involves a REST request for saving the post, and we wait for the response etc. And the callbacks registered with the __unstableSavePost hook typically want to do some additional async work related to saving. For example, the edit-post package registers a callback for saving metaboxes. I.e., in addition to the standard post saving, it will go through metabox forms on the page, and save their fields with a separate POST request to the server.
packages/editor/src/store/actions.js
Outdated
|
|
||
| if ( ! error ) { | ||
| await applyFilters( | ||
| 'editor.SavePost', |
There was a problem hiding this comment.
The other question is whether we want this to be stable or internal. Given the questioning about hook vs filter... I might think we should make it unstable first. WDYT?
There was a problem hiding this comment.
I'm fine with making it unstable and I'm mainly concerned about what are the right parameters for the action. is editor.__unstableSavePost a good name? Is there prior art for unstable/experimental actions and filters?
There was a problem hiding this comment.
editor.__unstableSavePost a good name
works for me.
Is there prior art for unstable/experimental actions and filters?
I don't know :) (At least, personally I don't remember any)
arguments
We can start with none, right?
There was a problem hiding this comment.
OK, renamed the action to editor.__unstableSavePost.
We can start with none, right?
I'm starting with options because I need to read options.isAutosave 🙂 This particular action doesn't need anything else, not even basic stuff like the post ID or the post edits.
jsnajdr
force-pushed
the
update/post-preview-button
branch
2 times, most recently
from
f2c7b36
to
5f9d573
Compare
| ); | ||
| } ); | ||
| export function isSavingPost( state ) { | ||
| return !! state.saving.pending; |
There was a problem hiding this comment.
My ultimate goal (probably never happens but who knows) is to make the editor package obsolete (because as an abstraction it's not clear). The change here goes in the opposite direction because it uses the store of this package. I think it's fine for now but I just wanted to share some thought on this.
There was a problem hiding this comment.
I think it's not just isSavingPost, but also adding a new functionality (the action hook) to the editor.savePost action goes in the opposite direction, too. When the savePost action moves somewhere else (to edit-post?), the isSavingPost selector and the state it reads will move as well.
As far as I know nothing is missing and once the tests come back green, the PR can be merged. |
|
A very relevant e2e test is failing right now:
I'll need to address that before merging. |
jsnajdr
force-pushed
the
update/post-preview-button
branch
2 times, most recently
from
327edae
to
4943da0
Compare
|
Looks like this fell through the cracks, I thought it was a nice refactor :) and might help in fixing some preview bugs. Should we try to update this, what was the blocker? |
There is one failing e2e test that relies on the details of the old implementation, and is hard to update. We'll probably have to remove the test. I didn't forget about this PR, I still think about it every few days 🙂 |
jsnajdr
force-pushed
the
update/post-preview-button
branch
from
4943da0
to
7376e29
Compare
|
I rebased this old PR onto latest Saving metaboxes, i.e., the The e2e test was relying on state that is now too internal to be reliably observable by a e2e test. If the CI checks pass, I believe this can finally be merged. |
|
If this makes its way to 6.3, it will need a dev note about the new |
mburridge
added
[Type] Code Quality
Yes, I'll document it. The best place seems to be the
I've been chatting with @lgladdy about using the new hook (and adding some new ones, like a |
|
Thank youyou, @jsnajdr! The discussions could also serve for sharing initial documentation and gathering feedback. Cc-ing @sc0ttkclark, just in case they're also interested in this discussion. |
|
If you get docs or have any discussions about this, I'd love to be a part of that. |
|
I see a few ACF references here, is there a working prototype example of how to leverage what's in this PR beyond documenting it which might take longer? I assume the ACF example will cover the use-cases I'm after as I'm doing something similar for Pods already. (reposting on my main account, sorry for the confusion) |
We discussed the specific ACF use case with @lgladdy some time ago, the discussion starts here: In short, Gutenberg should add also a new |
@jsnajdr Just to note, we shipped a fix for this in ACF 6.2.1 tonight, so hopefully debugging |
|
Pre save hook where we could do what ACF seeks to do for validation would be very helpful for other plugins too. |
|
Following up here, did a doc get posted on how to utilize this yet? |
|
Hey @sc0ttkclark I was following this thread as well and after testing the release, it doesn't solve the issue. I believe they only added filter because they needed it for metabox and not for plugins validation consideration. The filter was added to LINE 190 after the post has already been saved on line 157 So, even though it trigger an error if the filter third party plugin validation failed, it doesn't stop the posts from been published or updated. It's unfortunate we have to keep waiting for this. |
I tried adding that here (feedback welcome!): #58022 |
This PR extracts one part of the React 18 migration from #32765, the one where
PostPreviewButtonis rewritten to a functional component and awaits save/autosave instead of setting thepreviewWindow.locationin acomponentDidUpdatelifecycle method.The second commit ensures that the preview dropdown menu is closed after clicking the "preview in another tab" item. That's done with a new
onPreviewprop forPostPreviewButton, and passing theonCloserender prop fromDropDownMenudown throughPreviewOptionsto the button component.In current state, the PR has one major problem that needs to be addressed before it can be merged: if the post has active metaboxes, we need to wait for them to be finished saving before we can show the preview. But that waiting is now broken.
Originally, the
componentDidUpdatelifecycle waited for thepreviewLinkprop to change fromnullto a valid URL, and then set the preview location. With theforcePreviewLinkprop, which is being set tonullwhenisSavingMetaboxes()istrue, we were keeping thenullvalue for a while after the post save was finished, but that doesn't work now.Ideally, the "save metaboxes" action should hook into the
savePostandautosaveactions and be awaited there. That would replace the current listener that tries to catch the "save finished" event by checkingisSavingPosttransitions fromtruetofalse.