Hooks: add support for async filters and actions #64204
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,9 @@ import { | |
| removeAllActions, | ||
| removeAllFilters, | ||
| doAction, | ||
| doActionAsync, | ||
| applyFilters, | ||
| applyFiltersAsync, | ||
| currentAction, | ||
| currentFilter, | ||
| doingAction, | ||
|
|
@@ -943,3 +945,151 @@ 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? There was a problem hiding this comment. Added tests that verify that |
||
| test( 'runs all registered handlers', async () => { | ||
| addFilter( 'test.async.filter', 'callback_plus1', ( value ) => { | ||
| return new Promise( ( r ) => | ||
| setTimeout( () => r( value + 1 ), 10 ) | ||
| ); | ||
| } ); | ||
| addFilter( 'test.async.filter', 'callback_times2', ( value ) => { | ||
| return new Promise( ( r ) => | ||
| setTimeout( () => r( value * 2 ), 10 ) | ||
| ); | ||
| } ); | ||
|
|
||
| expect( await applyFiltersAsync( 'test.async.filter', 2 ) ).toBe( 6 ); | ||
| } ); | ||
|
|
||
| test( 'aborts when handler throws an error', async () => { | ||
| const sqrt = jest.fn( async ( value ) => { | ||
| if ( value < 0 ) { | ||
| throw new Error( 'cannot pass negative value to sqrt' ); | ||
| } | ||
| return Math.sqrt( value ); | ||
| } ); | ||
|
|
||
| const plus1 = jest.fn( async ( value ) => { | ||
| return value + 1; | ||
| } ); | ||
|
|
||
| addFilter( 'test.async.filter', 'callback_sqrt', sqrt ); | ||
| addFilter( 'test.async.filter', 'callback_plus1', plus1 ); | ||
|
|
||
| await expect( | ||
| applyFiltersAsync( 'test.async.filter', -1 ) | ||
| ).rejects.toThrow( 'cannot pass negative value to sqrt' ); | ||
| expect( sqrt ).toHaveBeenCalledTimes( 1 ); | ||
| expect( plus1 ).not.toHaveBeenCalled(); | ||
| } ); | ||
|
|
||
| test( 'is correctly tracked by doingFilter and didFilter', async () => { | ||
| addFilter( 'test.async.filter', 'callback_doing', async ( value ) => { | ||
| await new Promise( ( r ) => setTimeout( () => r(), 10 ) ); | ||
| expect( doingFilter( 'test.async.filter' ) ).toBe( true ); | ||
| return value; | ||
| } ); | ||
|
|
||
| expect( doingFilter( 'test.async.filter' ) ).toBe( false ); | ||
| expect( didFilter( 'test.async.filter' ) ).toBe( 0 ); | ||
| await applyFiltersAsync( 'test.async.filter', 0 ); | ||
| expect( doingFilter( 'test.async.filter' ) ).toBe( false ); | ||
| expect( didFilter( 'test.async.filter' ) ).toBe( 1 ); | ||
| } ); | ||
|
|
||
| test( 'is correctly tracked when multiple filters run at once', async () => { | ||
| addFilter( 'test.async.filter1', 'callback_doing', async ( value ) => { | ||
| await new Promise( ( r ) => setTimeout( () => r(), 10 ) ); | ||
| expect( doingFilter( 'test.async.filter1' ) ).toBe( true ); | ||
| await new Promise( ( r ) => setTimeout( () => r(), 10 ) ); | ||
| return value; | ||
| } ); | ||
| addFilter( 'test.async.filter2', 'callback_doing', async ( value ) => { | ||
| await new Promise( ( r ) => setTimeout( () => r(), 10 ) ); | ||
| expect( doingFilter( 'test.async.filter2' ) ).toBe( true ); | ||
| await new Promise( ( r ) => setTimeout( () => r(), 10 ) ); | ||
| return value; | ||
| } ); | ||
|
|
||
| await Promise.all( [ | ||
| applyFiltersAsync( 'test.async.filter1', 0 ), | ||
| applyFiltersAsync( 'test.async.filter2', 0 ), | ||
| ] ); | ||
| } ); | ||
| } ); | ||
|
|
||
| describe( 'async action', () => { | ||
| test( 'runs all registered handlers sequentially', async () => { | ||
| const outputs = []; | ||
| const action1 = async () => { | ||
| outputs.push( 1 ); | ||
| await new Promise( ( r ) => setTimeout( () => r(), 10 ) ); | ||
| outputs.push( 2 ); | ||
| }; | ||
|
|
||
| const action2 = async () => { | ||
| outputs.push( 3 ); | ||
| await new Promise( ( r ) => setTimeout( () => r(), 10 ) ); | ||
| outputs.push( 4 ); | ||
| }; | ||
|
|
||
| addAction( 'test.async.action', 'action1', action1 ); | ||
| addAction( 'test.async.action', 'action2', action2 ); | ||
|
|
||
| await doActionAsync( '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 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.
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 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.
The known use cases for
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 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 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: In PHP, a handler can return a 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:
Currently, this PR implements an equivalent of We could also add 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 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 Also, |
||
| } ); | ||
|
|
||
| test( 'aborts when handler throws an error', async () => { | ||
| const outputs = []; | ||
| const action1 = async () => { | ||
| throw new Error( 'aborting' ); | ||
| }; | ||
|
|
||
| const action2 = async () => { | ||
| outputs.push( 3 ); | ||
| await new Promise( ( r ) => setTimeout( () => r(), 10 ) ); | ||
| outputs.push( 4 ); | ||
| }; | ||
|
|
||
| addAction( 'test.async.action', 'action1', action1 ); | ||
| addAction( 'test.async.action', 'action2', action2 ); | ||
|
|
||
| await expect( doActionAsync( 'test.async.action' ) ).rejects.toThrow( | ||
| 'aborting' | ||
| ); | ||
| expect( outputs ).toEqual( [] ); | ||
| } ); | ||
|
|
||
| test( 'is correctly tracked by doingAction and didAction', async () => { | ||
| addAction( 'test.async.action', 'callback_doing', async () => { | ||
| await new Promise( ( r ) => setTimeout( () => r(), 10 ) ); | ||
| expect( doingAction( 'test.async.action' ) ).toBe( true ); | ||
| } ); | ||
|
|
||
| expect( doingAction( 'test.async.action' ) ).toBe( false ); | ||
| expect( didAction( 'test.async.action' ) ).toBe( 0 ); | ||
| await doActionAsync( 'test.async.action', 0 ); | ||
| expect( doingAction( 'test.async.action' ) ).toBe( false ); | ||
| expect( didAction( 'test.async.action' ) ).toBe( 1 ); | ||
| } ); | ||
|
|
||
| test( 'is correctly tracked when multiple actions run at once', async () => { | ||
| addAction( 'test.async.action1', 'callback_doing', async () => { | ||
| await new Promise( ( r ) => setTimeout( () => r(), 10 ) ); | ||
| expect( doingAction( 'test.async.action1' ) ).toBe( true ); | ||
| await new Promise( ( r ) => setTimeout( () => r(), 10 ) ); | ||
| } ); | ||
| addAction( 'test.async.action2', 'callback_doing', async () => { | ||
| await new Promise( ( r ) => setTimeout( () => r(), 10 ) ); | ||
| expect( doingAction( 'test.async.action2' ) ).toBe( true ); | ||
| await new Promise( ( r ) => setTimeout( () => r(), 10 ) ); | ||
| } ); | ||
|
|
||
| await Promise.all( [ | ||
| doActionAsync( 'test.async.action1', 0 ), | ||
| doActionAsync( 'test.async.action2', 0 ), | ||
| ] ); | ||
| } ); | ||
| } ); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious about why this is changed - is there a benefit to Set over [] or any risk this could break something? (eg is
addnot allowing duplicates a concern?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A hook run must be able to reliable add and remove itself to
__currentwhen it starts and ends. With async hooks, multiple hooks can run at the same time, independently and in parallel, and array.pushand.popis no longer reilable. You could be.popping not your own record, but someone else's. That gets fixed with aSet(of currently running hooks).