Core Data: Add __experimentalBatch() #28210
Merged
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
noisysocks
added
REST API Interaction
noisysocks
commented
Jan 15, 2021
noisysocks
commented
Jan 15, 2021
noisysocks
commented
Jan 15, 2021
noisysocks
commented
Jan 15, 2021
|
Size Change: -1.64 kB (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
noisysocks
commented
Jan 15, 2021
talldan
reviewed
Jan 15, 2021
| } ) ); | ||
| } | ||
|
|
||
| return batchResponse.responses.map( ( response ) => { |
There was a problem hiding this comment.
It'd be nice if those also checked against request.parse and when false created the full response object.
There was a problem hiding this comment.
Sorry not sure what you mean here?
|
I like how it's more succinct, it's much easier to follow without going through the store for data. What I don't like is that we're losing the power of redux devtools. Maybe it could dispatch redux actions without any reducers as a compromise solution? There would be at least some visibility as to what's happening. |
|
Not sure that it makes sense to dispatch an action if there's no reducer or control that does anything with it? I agree with you though that this stuff is a pain to debug. It's because rungen adds a lot of indirection to the call stack which makes it difficult to use DevTools. I'd probably prefer that we fix that by moving away from rungen/controls to async functions i.e. #27276. |
|
I'm fairly happy with this. Just working on tests and documentation now and then I'll un-draft the PR. I don't really like the export function* __experimentalBatch( tasks ) {
const batch = createBatch();
const dispatch = yield getDispatch();
const api = { ... };
const results = tasks.map( ( task ) => task( api ) );
yield __unstableAwaitPromise( batch.run() );
return yield __unstableAwaitPromise( Promise.all( results ) );
}But I'm hopeful that something will come of #27276 which will let us remove both of those: export const __experimentalBatch = ( tasks ) => async ( { dispatch } ) => {
const batch = createBatch();
const api = { ... };
const results = tasks.map( ( task ) => task( api ) );
await batch.run();
return await Promise.all( results );
}; |
noisysocks
force-pushed
the
try/simpler-batch-processing
branch
from
9167ac8 to
0a96262
Compare
noisysocks
requested review from
ajitbohra,
chrisvanpatten,
draganescu,
kevin940726,
mkaz,
nerrad and
tellthemachines
as code owners
noisysocks
changed the title
|
All ready! I added tests, wrote some docs, and updated the PR description. |
jsnajdr
reviewed
Jan 19, 2021
|
Haven't looked into the diff yet, but after testing locally, seems like this PR fixes #28186 as well! 🎉 |
jsnajdr
reviewed
Jan 19, 2021
noisysocks
force-pushed
the
try/simpler-batch-processing
branch
from
c3e67b3 to
5a9c126
Compare
kevin940726
reviewed
Jan 25, 2021
noisysocks
added
the
[Type] New API
- Replaces the batch-processing data store with a stripped back API that doesn't use any persistence. - Adds __experimentalBatch() to core-data. - Changes edit-widgets to use __experimentalBatch(). - Fixes batch processing race condition by using batch.add( thunk ).
noisysocks
force-pushed
the
try/simpler-batch-processing
branch
from
34db07a to
90535c1
Compare
This was referenced Jan 28, 2021
|
Is this respecting the batch size limit? See https://make.wordpress.org/core/2020/11/20/rest-api-batch-framework-in-wordpress-5-6/ |
|
No, oops. I'll turn your comment into an issue. |
7 tasks
dmsnell
reviewed
Mar 15, 2022
| * | ||
| * - The thunk calls its `add` argument, or; | ||
| * - The thunk returns a promise and that promise resolves, or; | ||
| * - The thunk returns a non-promise. |
There was a problem hiding this comment.
@noisysocks can you clarify the intention of the second and third options here? it seems to suggest that we can do this…
batch.add( () => Promise.resolve({ path: '/v1/books', ... }) );or this…
batch.add( () => ({ path: '/v1/books', ... }) );…however, if we do that, then Promise.resolve(fetchOptionsProducer( queueForResolution)) will return the API request description and not the data requested in the batch operation.
- Am I misunderstanding the comment?
- Did we intend on eventually providing this functionality?
- Is this an un-noticed bug in the code?
I'm working on the types for this module and got stuck trying to understand the purpose here. I'm inclined to remove the second and third options since the only examples and existing core code only demonstrate the first.
There was a problem hiding this comment.
…however, if we do that, then
Promise.resolve(fetchOptionsProducer( queueForResolution))will return the API request description and not the data requested in the batch operation.
I'm not sure what you mean here. What's fetchOptionsProducer?
From memory this was all about preventing run() from pausing infinitely if add() is called with a thunk and that thunk never calls the function passed to it.
The comment is describing the effect of this code below:
return Promise.resolve( inputOrThunk( add ) ).finally( () => {
pending.delete( id );
} );
- The thunk calls its
addargument, or;
const batch = createBatch();
batch.add( ( add ) => async {
await sleep( 100 );
add( {} ); // run() will resume here
await sleep( 100 );
} );
batch.run(); // will pause for 100 ms
- The thunk returns a promise and that promise resolves, or;
const batch = createBatch();
batch.add( ( add ) => async {
await sleep( 100 );
await sleep( 100 );
// run() will resume here when promise resolves
} );
batch.run(); // will pause for 200 ms
- The thunk returns a non-promise.
const batch = createBatch();
batch.add( ( add ) => {
// run() will resume here as a non-promise is returned
} );
batch.run(); // will pause for 0 msI'm working on the types for this module
I think it'd be something like this.
interface Batch<TInput, TOutput> {
add<TThunkResult>( inputOrThunk: TInput|( ( add: ( input: TInput ) => Promise<TOutput> ) => TThunkResult|Promise<TThunkResult> ) ): Promise<TOutput>|Promise<TThunkResult>;
}
function createBatch<TInput, TOutput>( processor: ( inputs: TInput[] ) => Promise<{output?: TOutput, error?: any}[]> ): Batch<TInput, TOutput> {
}I'm inclined to remove the second and third options since the only examples and existing core code only demonstrate the first.
Feel free to simplify! It's __experimental so should be okay to change. I believe the unit tests and usage in @wordpress/core-data cover everything we need createBatch to do.
| reject( result.error ); | ||
| isSuccess = false; | ||
| } else { | ||
| resolve( result?.output ?? result ); |
There was a problem hiding this comment.
this seems to be a violation of the contract we provide for the output Promise. It seems possible that we added this so we could write tests that violate the input contract.
I noticed that the tests pass values like 1 and 2 and we test processors that return non-object. Are these intentionally valid use-cases of the function? if so, what are the reasons for allowing that? should we require that alternative processors conform to the { output } | { error } structure?
Do we know of any alternative processors in use (apart from the core tests here)?
There was a problem hiding this comment.
Good catch, this is an undocumented shorthand for specifying a result.
I'm fine with updating the tests and removing the shorthand so that this is simply resolve( result.output ).
Alternatively, processor could have a type of ( inputs: TInput[] ) => Promise<({output: TOutput}|{error: any}|TOutput)[]>. Why make it hard for ourselves though? 😀
Do we know of any alternative processors in use (apart from the core tests here)?
I believe only the default processor is used by Gutenberg. I'd be surprised if third parties have used this API as it's pretty niche.
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.
Fixes #27173.
Fixes #28186.
Alternative to #27241.
Replaces the
batch-processingdata store with a stripped back API based on https://github.com/WordPress/gutenberg/pull/27241/files#r555573124 that doesn't use any persistence.Adds
__experimentalBatch()to core-data.Changes
saveWidgetArea()to use__experimentalBatch().Properly fixes Widgets editor: "There was an error. object Object" on save #27173 by ensuring that
batch.run()waits for all inputs to be added before proceeding.@youknowriad @TimothyBJacobs @kevin940726 @talldan
A ✅ should be sufficient for testing as until tests are included and
adding-widgets.test.jsincludes a test for saving widgets.