AJAX Requests to loop over checks to run#114
Conversation
|
Set up as a draft so once #104 is merged I can update with the JS formatting here |
| for (var i = 0; i < data.checks.length; i++) { | ||
| let result = await runCheck( data.plugin, data.checks[ i ] ); | ||
| results.push( result ); | ||
| } |
There was a problem hiding this comment.
As a rule, it is a not good to do a series of promises in a loop like this. Do these NEED to run it order. What if one of the promises fail?
There was a problem hiding this comment.
I would recommend using Promise.all here.
Try something like this.
const promises = [];
for (var i = 0; i < data.checks.length; i++) {
promises.push( runCheck( data.plugin, data.checks[ i ] ) );
}
const results = await Promise.all( promises );
There was a problem hiding this comment.
Hmm, not sure about that one here. Normally I would agree with you, but I think it's a better user experience to show the check results in a consistent order, so circling through the checks makes sense to me in this context.
If we use Promise.all, they will be handled in a random order. That would only be a good idea if we wanted to output all results together at the end, but I think it's a good idea to output results one by one as it gives the user feedback already as soon as there are results.
There was a problem hiding this comment.
Thanks @spacedmonkey @felixarntz
Another point to raise here is that we would want these to run sequentially. We wouldn't want to have 2 runtime checks running in parallel, where both their preparations are implemented at the same time, as this may cause incorrect results returned by checks.
I have updated this to log the results as they're returned.
| // If there are any files left with only warnings, print those next. | ||
| foreach ( $warnings as $file_name => $file_warnings ) { | ||
| $file_results = $this->flatten_file_results( $file_name, array(), $file_warnings ); | ||
| $all_results = array_merge( $all_results, $file_results ); |
There was a problem hiding this comment.
It can be expensive to call array_merge. Maybe just add $file_results to an array and then fatten it at the end.
There was a problem hiding this comment.
Thanks @spacedmonkey I agree here, although the updates to handle the formatting on the JS side now has meant this code has been removed.
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
Thanks @jjgrainger, mostly looks good. My main feedback is that I don't think we should do all the formatting in the AJAX endpoint as it seems unnecessary here and could happen in JS depending on the template usage.
| for (var i = 0; i < data.checks.length; i++) { | ||
| let result = await runCheck( data.plugin, data.checks[ i ] ); | ||
| results.push( result ); | ||
| } |
There was a problem hiding this comment.
Hmm, not sure about that one here. Normally I would agree with you, but I think it's a better user experience to show the check results in a consistent order, so circling through the checks makes sense to me in this context.
If we use Promise.all, they will be handled in a random order. That would only be a good idea if we wanted to output all results together at the end, but I think it's a good idea to output results one by one as it gives the user feedback already as soon as there are results.
|
Thanks @spacedmonkey & @felixarntz I've made updates based on the feedback and is ready for you to review. |
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@jjgrainger Thank you for the updates, LGTM!
One tiny thing unrelated to the actual PR.
spacedmonkey
left a comment
spacedmonkey
left a comment
There was a problem hiding this comment.
Requested one change and I will approve. We are nearly there.
3 participants
Addresses acceptance criteria in #84
Closes #84