Refactor: Remove post actions hook. #61040
Closed
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
jorgefilipecosta
added
the
[Type] Code Quality
|
Size Change: +135 B (0%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
mcsf
reviewed
Apr 24, 2024
There was a problem hiding this comment.
Preliminary thoughts:
- Overall, an improvement for sure. Seems like the right direction, so don't take my next comments as blockers.
- On one hand, I like the "transparency" of dealing with a simple structure, like the
actionobject and theaction.callbackfunction. On the other hand, it doesn't feel great to need to do the thunk-like juggling (if ( typeof returnResult === 'function' ) {) across three different files whenever we call the callback.- That makes me wonder if we'll want to delegate the actual calling of the callback to some central system (but which? something like
editorActions.dispatch( action )?) - I was so thirsty for a more consistent solution that I even wondered — and forgive the blasphemy — if we'd be better off with actions as class instances (
class EditPostAction extends Action)? 🙈 But this would disrupt the convenience of wrapping functions (onActionPerformed: ( ...args ) => {), forget it.
- That makes me wonder if we'll want to delegate the actual calling of the callback to some central system (but which? something like
- Even as I type this, I realise that I'm mixing up
action.onActionPerformedandaction.callbackthroughout the diff. That's something to work on. - And speaking of confusion, even though it doesn't come from this PR, I can't help but notice that we are setting up our readers for confusion in a few subtle ways, like naming:
- In a file where there are two main "entities" (Data Views items and actions), we see identifiers like
BulkActionItem. I have to think twice whenever I return to this branch. - I also sense we are repeating work in some places, like how some cascading components each compute
eligibleItems.
- In a file where there are two main "entities" (Data Views items and actions), we see identifiers like
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Thank you a lot for the review @mcsf!
It is definitively not great, but that is internal for an extender that complexity is not seen. It is a thunk-like implementation. I will try to make it more abstracted with a function executeAction( action ) that may be reaped on two places dataviews and editor but at least that complexity is localized and not on click handlers.
This mechanism needs to be shared between the dataviews and editor package and it is not obvious the package where it could reside.
What do you think we can do here? Naming changes or do you have another improvement in mind?
It seems we have a case where it happens and computation of eligible items could be reused in packages/dataviews/src/bulk-actions.js. I will address this in a parallel PR if there is other cases let me know. |
|
Would it make sense to have a It could have a special flag for core actions that can't be removed, but developers could add, remove or extend actions. This would also be useful for requests like #60637 (comment) and #61083. |
mcsf
added a commit
that referenced
this pull request
May 6, 2024
Motivation ========== Actions and commands -------------------- In the context of Data Views, there has been a lot of recent work towards providing a set of actions operating on posts, templates, patterns (e.g. rename post, edit post, duplicate template), and ultimately other entities. These actions, however, aren't unique to Data Views, and indeed exist in several different contexts (e.g. Site Editor inner sidebar, new Admin "shell" sidebar, Pages index view, Post Editor), so the next step was to unify actions across packages (e.g. #60486, #60754). The first unification effort led to an abstraction around a hook, `usePostActions`, but the consensus now is to remove it and expose the actions directly (#61040). Meanwhile, it has been noted that there is a strong parallel between these _actions_ and the Command Palette's _commands_, which has its own API already. This isn't a 1:1 mapping, but we should determine what the overlap is. Actions and side effects ------------------------ There is a limit to how much we can unify, because the context in which actions are triggered will determine what secondary effects are desired. For example, trashing a post inside the post editor should result in the client navigating elsewhere (e.g. edit.php), but there should be no such effect when trashing from a Data View index. The current solution for this is to let consumers of the `PostActions` component pass a callback as `onActionPerformed`. It works but there's a risk that it's too flexible, so I kept wondering about what kind of generalisations we could make here before we opened this up as an API. Extensibility ------------- As tracked in #61084, our system -- what ever it turns to be -- needs to become extensible soon. Somewhere in our GitHub conversations there was a suggestion to set up an imperative API like `registerAction` that third parties could leverage. I think that's fair, though we'll need to determine what kind of registry we want (scope and plurality). An imperative API that can be called in an initialisation step rather than as a call inside the render tree (e.g. `<Provider value=...>` or `useRegisterAction(...)`) is more convenient for developers, but introduces indirection. In this scenario, how do we implement those aforementioned _contextual side effects_ (e.g. navigate to page)? The experiment ============== It was in this context that I had the terrible thought of leveraging wp.hooks to provide a private API (to dogfood in Gutenberg core packages). But, of course, hooks are keyed by strings, and so they are necessarily public -- i.e., a third party can call `applyFilters('privateFilter'`, even if `privateFilter` is not meant to be used outside of core. This branch changes that assumption: hook names *must* be strings, *except* if they match a small set of hard-coded symbols. These symbols are only accessible via the lock/unlock API powered by the `private-apis` package. Thus, core packages can communicate amongst each other via hooks that no third party can use. For example: - An action triggers `doAction` with a symbol corresponding to its name (e.g. `postActions.renamePost`). - A consumer of actions, like the Page index view (PagePages), triggers a more contextual action (e.g. `pagePages.renamePost`). - A different component hooks to one of these actions, according to the intended specificity, to trigger a side effect like navigation. See for yourself: upon `pagePages.editPost`, the necessary navigation to said post is triggered by a subscriber of that action. Assessment ========== Having tried it, I think this is a poor idea. "Private hooks" as a concept is a cool way to see how far `private-apis` can take us, but they seem like the wrong tool for the current problem. Still, I wanted to share the work, hence this verbose commit. I think our next steps should be: - Finish the actions refactor (#61040) - Impose constraints on ourselves to try to achieve our feature goals with less powerful constructs than `onActionPerformed`. I'm still convinced we haven't done enough work to generalise side effects. Consider it along with the commands API. - Try a more classic registry-based approach for actions (`registerAction`)
|
Closing this PR as we end up going another path at #61520. |
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.
As a follow-up to the discussions that happened at #60754 (review).
This PR does the following refactors:
cc: @youknowriad, @ntsekouras.
Testing Instructions
I tried to use the the actions on dataviews (pages, templates on patterns) as single action and as bulk actions and verified things work.
I tried to use actions on the post editor for a post and on the site editor inspector and details panel for a page and checked thins still work.