Skip to content

PostPreviewButton: rewrite to functional, avoid state transitions in lifecycles #44971

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.

Sign up for GitHub

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

Merged
jsnajdr merged 9 commits into from
Jul 10, 2023
Merged

PostPreviewButton: rewrite to functional, avoid state transitions in lifecycles #44971

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9b9d188
PostPreviewButton: rewrite to functional, avoid state transitions in …
jsnajdr Oct 13, 2022
56f6c59
PreviewOptions: close dropdown after opening external preview
jsnajdr Oct 14, 2022
f524989
Move metaboxes save into a hook in savePost
jsnajdr Oct 19, 2022
92eda2f
Make isSavingPost true during entire save, not only when saving the e…
jsnajdr Oct 19, 2022
df7ff3e
Remove forceIsSaving and forcePreviewLink props: isSavingPost returns…
jsnajdr Oct 19, 2022
e00da35
Fix unit tests for isEditedPostAutosaveable and isSavingPost
jsnajdr Oct 20, 2022
69e8c5c
Fix PostPreviewButton unit tests
jsnajdr Oct 21, 2022
7376e29
Rename the action to editor.__unstableSavePost
jsnajdr Oct 21, 2022
df880bc
Remove the 'disabled when saving metaboxes' e2e test
jsnajdr Jul 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Rename the action to editor.__unstableSavePost
  • Loading branch information
@jsnajdr
jsnajdr committed Jul 10, 2023
commit 7376e295061ff49e7cbcc685b063797821fd37bc
2 changes: 1 addition & 1 deletion packages/edit-post/src/store/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -570,7 +570,7 @@ export const initializeMetaBoxes =

// Save metaboxes on save completion, except for autosaves.
addFilter(
'editor.SavePost',
'editor.__unstableSavePost',
'core/edit-post/save-metaboxes',
( previous, options ) =>
previous.then( () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/editor/src/store/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ export const savePost =

if ( ! error ) {
await applyFilters(
Copy link
Contributor

@youknowriad youknowriad Oct 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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' )

Copy link
Member Author

@jsnajdr jsnajdr Oct 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@adamsilverstein adamsilverstein Aug 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@jsnajdr jsnajdr Aug 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

'editor.SavePost',
'editor.__unstableSavePost',
Promise.resolve(),
options
).catch( ( err ) => {
Expand Down