Hooks: add support for async filters and actions #64204
Conversation
|
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. |
|
Size Change: +107 B (+0.01%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
| @@ -943,3 +944,17 @@ test( 'checking hasFilter with named callbacks and removeAllActions', () => { | |||
| expect( hasFilter( 'test.filter', 'my_callback' ) ).toBe( false ); | |||
| expect( hasFilter( 'test.filter', 'my_second_callback' ) ).toBe( false ); | |||
| } ); | |||
|
|
|||
| test( 'async filter', async () => { | |||
There was a problem hiding this comment.
This looks great. :)
Can we have an example with actions?
There was a problem hiding this comment.
Yes, if you like it I'll finish the PR: add a full test suite and documentation 🙂
There was a problem hiding this comment.
I added a few more tests. I'd also like to add more documentation, but the hooks package is not very well documented in the first place. It's like if it was assumed that everyone already knows how WordPress actions and filters work. There are *.md guides documenting specific actions and filters, but the API itself is not documented. Except some jsdoc comments which are however not propagated to any *.md docs.
Should we add this note in the |
| addAction( 'test.async.action', 'action2', action2 ); | ||
|
|
||
| await doAsyncAction( 'test.async.action' ); | ||
| expect( outputs ).toEqual( [ 1, 2, 3, 4 ] ); |
There was a problem hiding this comment.
Actually, I wonder if this should be the right order. I feel like the correct order should be 1, 3, 2, 4 (in other words, internally we'd use Promise.all to allow for parallel actions).
There was a problem hiding this comment.
I feel like the order doesn't matter in actions (as opposed to filters)
There was a problem hiding this comment.
addAction also has priority as argument, both in JS and PHP versions. So the order matters and can be influenced.
The only difference between actions and filters is that filters process a "value" that is passed from one handler to another.
There was a problem hiding this comment.
Yes true but I feel that for most actions, we don't really care about the order. I wonder if it should be an option. For instance, this has the waterfall effect if multiple plugins trigger REST APIs for instance in an async action...
Anyway, I can leave with both approaches.
There was a problem hiding this comment.
Now I understand, the registered actions could also run in parallel, and there would be one Promise.all or Promise.allSettled that waits and only then doAsyncAction returns.
Why not, but that means entering a new territory where there is not precedent with existing PHP and JS actions. These all run sequentially.
Do you have a use case for async actions? Filters are easy, they always must be sequential, but actions can run in different modes.
There was a problem hiding this comment.
Theoretically, order should matter for both actions and filters, specifically if we want to mirror the hook behavior on the PHP side. Order does matter there, and is predictable. It's important for consistent behavior when you have multiple functions hooked to the same action.
Imagine you're hooking 10 functions from separate plugins, each of them registering tabs in a tabbed layout. If you care about the tab order (and you likely do), then you'll likely care about the consistency of the action order.
There was a problem hiding this comment.
think about the "post save" action we have or the "pre save" action. How do you see plugins using this one?
The known use cases for preSavePost are:
- validation. E.g., the ACF plugin does its own form validation and aborts the save early if it fails: PostPreviewButton: rewrite to functional, avoid state transitions in lifecycles #44971 (comment)
- modifying the
edits, e.g., to calculate some derived fields from modified ones. Block hooks could use this, but it's not yet an use case that's been validated by actually implementing it.
Validations can run in parallel, because the result is a boolean success/fail. One failed validation is sufficient to fail them all.
Modifying edits must run sequentially, the output of one handler is fed to the next one.
For savePost (after the entity was saved to REST) the known use case is saving metaboxes. After the post itself is saved, the metabox forms are saved to _wpMetaBoxUrl. These handlers could run in parallel.
After some consideration, I believe that async actions are inherently parallel, because the input of any action doesn't depend on the output of any other. While async filters are inherently sequential.
I'm not sure about handling errors. Until now the sync hook didn't really support throwing errors. Stale data would be left inside hooksStore.__current if any handler threw an exception, there was no try/catch block.
In the current state of this PR, throwing an error immediately aborts the hook run. Further handlers are called only when all previous handlers succeed. It's not possible, for example, that a later handler recovers from an error thrown by an earlier one. Like you can do with promises: promise.then().catch().then(). I'm not sure if this lack of flexibility is a problem or not.
In PHP, a handler can return a wp_error and the next handler has complete freedom what to do with that error.
There was a problem hiding this comment.
Webpack has a very sophisticated system for various kinds of sync and async hooks, published separately as the tapable package. These are the kinds of async hooks that they provide:
AsyncSeriesHook: all handlers run one at at time, in a sequence. If any handler throws an error, further handlers are not called and the hook run finishes with that error. All handlers receive the same parameters (passed to the "run" function) and don't return anything. The return type of handlers and the "run" function isPromise<void>.AsyncSeriesBailHook: a slight variation of the previous hook where a handler can abort the process not only by throwing an error, but also by returning a non-undefinedvalue. If a handler has "something to say", it will return that, and it will become the result of the hook run. The same logic as with errors, but the semantic is not an error. The hook can have both a regular result and an error. The return type isPromise<T | undefined>.AsyncSeriesWaterfallHook: here each handler's first argument is the return value of the previous handler. The remaining argument are constant. Very similar to a WordPress filter.AsyncParallelHook: all handlers are running in parallel, started at the same time. If any of them throws an error, the hook run finishes with that error. The other handlers still run (we can't abort them) but their return values will be ignored. Very similar toPromise.all.AsyncParallelBailHook: all handlers also run in parallel, and if any of them bails (returns non-undefinedvalue) or throws, that becomes the result of the hook run. Other handlers continue to run but their results are ignored.
Currently, this PR implements an equivalent of AsyncSeriesHook (doAsyncAction) and AsyncSeriesWaterfallHook (applyAsyncFilters).
We could also add AsyncParallelHook, to do an action in parallel. Could be a parameter of doAsyncAction (parallel = true | false), but there is no good way to distinguish it from other parameters. So a better option is doAsyncActionParallel method.
The "bail" logic doesn't have any prior art in WordPress and I think we don't need it. It can be always simulated by each handler doing a check:
if ( value !== undefined ) {
return value;
}
// start doing the actual jobThere are WordPress filters, like determine_current_user, that are of the "bail" type and do exactly this.
So, what do you think? How much inspiration should we take here?
There was a problem hiding this comment.
I'm still hesitant about which one do we need more parallel or not but I'm thinking that we should pick one for now and see how far we can get with it.
Your call, if you think "serial/waterfall" is better as default, we can go with it for now and consider a separate function later for parallel if need be.
There was a problem hiding this comment.
Looking at the base webpack codebase, they have 42 async hook, and only 6 of them are parallel. The majority, 36, are serial. That suggests that parallel actions are an exception.
So, yes, I'll go with doActionAsync and applyFiltersAsync being serial by default. We can add doActionAsyncParallel at any time later when there is a demand.
Also, doActionAsync seems to be a better name than doAsyncAction. It follows a convention where the base name is doAction and there is a series of modifiers. Core has do_action_deprecated or do_action_ref_array, so this convention is already established. I'll modify the PR accordingly.
There was a problem hiding this comment.
Nice work @jsnajdr 👍
I think this could definitely use some more work on the documentation side. Folks coming from the backend world of WP might be confused as to why we have two different types of functions for executing the hooks. Maybe some examples could help with that.
packages/hooks/CHANGELOG.md
Outdated
| @@ -2,6 +2,10 @@ | |||
|
|
|||
| ## Unreleased | |||
|
|
|||
| ### Enhancements | |||
There was a problem hiding this comment.
Aren't these a new feature?
| ### Enhancements | |
| ### New Features |
packages/hooks/CHANGELOG.md
Outdated
| @@ -2,6 +2,10 @@ | |||
|
|
|||
| ## Unreleased | |||
|
|
|||
| ### Enhancements | |||
|
|
|||
| - added new `doAsyncAction` and `applyAsyncFilters` functions to run hooks in async mode ([#64204](https://github.com/WordPress/gutenberg/pull/64204)). | |||
There was a problem hiding this comment.
| - added new `doAsyncAction` and `applyAsyncFilters` functions to run hooks in async mode ([#64204](https://github.com/WordPress/gutenberg/pull/64204)). | |
| - added new `doAsyncAction` and `applyAsyncFilters` functions to run hooks in async mode ([#64204](https://github.com/WordPress/gutenberg/pull/64204)). |
|
|
||
| /** @type {import('.').Store} filters */ | ||
| this.filters = Object.create( null ); | ||
| this.filters.__current = []; | ||
| this.filters.__current = new Set(); |
There was a problem hiding this comment.
I had some concerns about switching the storage type, worried that someone could be using the internals (using the internals for the PHP version of the hooks is not uncommon), but I didn't spot any custom usage, so should be fine.
There was a problem hiding this comment.
Curious about why this is changed - is there a benefit to Set over [] or any risk this could break something? (eg is add not allowing duplicates a concern?
There was a problem hiding this comment.
A hook run must be able to reliable add and remove itself to __current when it starts and ends. With async hooks, multiple hooks can run at the same time, independently and in parallel, and array .push and .pop is no longer reilable. You could be .popping not your own record, but someone else's. That gets fixed with a Set (of currently running hooks).
| @@ -943,3 +945,86 @@ test( 'checking hasFilter with named callbacks and removeAllActions', () => { | |||
| expect( hasFilter( 'test.filter', 'my_callback' ) ).toBe( false ); | |||
| expect( hasFilter( 'test.filter', 'my_second_callback' ) ).toBe( false ); | |||
| } ); | |||
|
|
|||
| describe( 'async filter', () => { | |||
There was a problem hiding this comment.
Should we also add tests that verify the functions that work with the current hook work well with async filters/actions? doingAction comes to mind.
There was a problem hiding this comment.
Added tests that verify that doingAction/doingFilter return correct results when multiple hooks are running at once, in parallel.
| addAction( 'test.async.action', 'action2', action2 ); | ||
|
|
||
| await doAsyncAction( 'test.async.action' ); | ||
| expect( outputs ).toEqual( [ 1, 2, 3, 4 ] ); |
There was a problem hiding this comment.
Theoretically, order should matter for both actions and filters, specifically if we want to mirror the hook behavior on the PHP side. Order does matter there, and is predictable. It's important for consistent behavior when you have multiple functions hooked to the same action.
Imagine you're hooking 10 functions from separate plugins, each of them registering tabs in a tabbed layout. If you care about the tab order (and you likely do), then you'll likely care about the consistency of the action order.
Maybe we could deprecate or outright remove them. A wpdirectory.net search showed that it's not used by any public plugin. |
jsnajdr
force-pushed
the
add/async-hooks
branch
from
75d3f94 to
a04f234
Compare
jsnajdr
force-pushed
the
add/async-hooks
branch
from
a04f234 to
36ffa94
Compare
|
This PR is now ready for final review and merging, but there is one big problem: we missed the deadline for 6.7 enhancements, only bugfixes can be backported now. It wouldn't be a big deal but this PR is a prerequisite for stabilizing the new @youknowriad @noisysocks Is there still a chance to backport this enhancement before beta 1? |
It's true that the new hooks are not documented -- because the hooks package as a whole doesn't have any documentation at all. It's like there was an assumption at the beginning that everyone knows what hooks are, because it's a major WordPress PHP feature. This can be fixed, of course, but probably in another PR that documents the package as a whole. |
jsnajdr
force-pushed
the
add/async-hooks
branch
from
36ffa94 to
0e4dec8
Compare
jsnajdr
added
the
Backport to WP 6.7 Beta/RC
github-actions
bot
removed
the
Backport to WP 6.7 Beta/RC
* Hooks: add support for async filters and actions * Unit tests for doing/didAction/Filter
github-actions
bot
added
the
Backported to WP Core
|
I just cherry-picked this PR to the wp/6.7 branch to get it included in the next release: f42a8d0 |
fabiankaegy
added
the
Needs Dev Note
|
Hey @jsnajdr 👋 Would you be able to help write a dev note for this for the 6.7 release? We are planning to have this as part of a larger Miscellaneous Editor Updates note. We are hoping to get all drafts in by October 13th to leave some time for reviews before the RC1. All Dev Notes get tracked in #65784 so feel free to leave a note there or ping me directly :) Please let us know if you can assist with that. Thanks in advance :) |
|
Hi @fabiankaegy 👋 Yes, I'm going to write the dev note. |
Adds support for async actions and filters, via new
applyAsyncFiltersanddoAsyncActionmethods.You add an async function as a filter: its input is a synchronous value (the hook runner ensures that) and the output can be a promise:
With async actions the
currentActionandcurrentFilterfunctions lose most of their meaning: they now return the hook that started running last and is still running. Although the other currently running hooks are also "current".@youknowriad please try it out 🙂