Propose batch saveEntityRecords action #27241
Conversation
adamziel
requested review from
ajitbohra,
chrisvanpatten,
draganescu,
kevin940726,
mkaz,
nerrad,
noisysocks,
talldan and
tellthemachines
as code owners
adamziel
added
[Package] API fetch
|
Size Change: -9.19 kB (0%) Total Size: 1.19 MB
ℹ️ View Unchanged
|
|
I think we still need to preflight the batch endpoint to get the max items value. |
|
You got it @TimothyBJacobs! Added in 774c3df. |
|
cc @ellatrix @gziolo @youknowriad I'd love to give more polish to this PR (like docstrings), but it would be good to hear your preliminary feedback about the direction first :-) |
| */ | ||
| export function* __experimentalBatchSaveEntityRecords( spec ) { | ||
| for ( const { kind, type, key, record } of spec ) { | ||
| if ( key ) { |
There was a problem hiding this comment.
This check seems weird, why sometimes we have a key and other times a record
There was a problem hiding this comment.
If we're editing an existing record then there is a key. If we're creating a new record then there is no key.
| } | ||
| } | ||
| yield __unstableAwaitPromise( | ||
| new Promise( ( resolve ) => setTimeout( resolve, 10 ) ) |
There was a problem hiding this comment.
Why do we need a random timeout here?
There was a problem hiding this comment.
My guess is that it addresses an issue similar to what I outlined in #27173 (comment). Rungen hands execution back to __experimentalBatchSaveEntityRecords which calls processBatch before addToBatch has been called.
Probably we should replace this setTimeout with something akin to await waitUntilNItemsAreEnqueued( spec.length ).
|
|
||
| // Ensure batch-processing store is available for dispatching | ||
| setTimeout( () => { | ||
| dispatch( 'core/__experimental-batch-processing' ).registerProcessor( |
There was a problem hiding this comment.
This is global and applies to all registries, it shouldn't be the case. I think the issues comes from the "registerProcessor" API. Why do we need to register a processor at all and why are these tracked in the store.
There was a problem hiding this comment.
Sorry, picking this work up from @adamziel and not sure what you mean here @youknowriad! What you would replace registering a processor with?
There was a problem hiding this comment.
@adamziel wrote some docs on @wordpress/batch-processing here:
My understanding is that the processor concept makes it so that @wordpress/batch-processing is a generic way to batch async work and not something that's reliant on /v1/batch.
I'm not against removing this abstraction and instead calling apiFetch( '/v1/batch' ) directly from saveEntityRecords. In general I think it's better to hold off on adding abstractions until we're sure that we need it. Is this what you mean @youknowriad?
There was a problem hiding this comment.
Hey @noisysocks my comment was not clear it's not the abstraction that bothers me but more the shape of the API (relying on a store and strings).
For example, an API like that would be more appealing to me:
// Create the "processor"
const processor = createProcessor( inputs => doSomethingWithInputs );
// Create the "batch" to be kept in core-data if it's something that need to persist between action calls
// or just use a local variable if the batching happens within the same function like desired here `__experimentalBatchSaveEntityRecords`
const batch = createBatch( processor );
batch.add( input )
batch.add( input2 )
const result = batch.flush()
There was a problem hiding this comment.
Mmm! Very good. I'll have a play.
|
|
Description
Batch requests are now supported in WP core and already used in
edit-widgetsGutenberg package. They could enhance any code issuing a burst of requests. Consider multi-entity saving:gutenberg/packages/editor/src/components/entities-saved-states/index.js
Lines 112 to 114 in 0ab323a
The code above issues one request per entity record. Wouldn't it be nice if it would issue just one request and save multiple entity records at once? Many pieces of that puzzle are already in their place, and this PR would take us one step closer by exposing the following API:
From here, the last missing piece would be enabling batch requests in all the API endpoints used in the site editor.
Notes
async/awaitinstead of using generators, controls, yields, and alike. Once that happens, the cleanup here should be easier by an order of magnitude.How has this been tested?
Types of changes
New feature (non-breaking change which adds functionality)
Checklist: