Website query API: Continue plugin installs on error#605
Website query API: Continue plugin installs on error#605eliot-akira wants to merge 19 commits intoWordPress:trunkfrom
Conversation
…omplete even if any fails - Resolves WordPress#600
|
so fast @eliot-akira! thanks - I might need a few days or more to review this. |
|
Oops, when I added you as reviewer, it said "Request review" but this isn't ready for review yet. It's just a rough draft to get started, I haven't checked if this solves the issue. I'm making some comments to think about how to best implement the solution, and will develop it further. I still have some doubts, so any feedback is welcome. When it's more in shape I'll let you know by switching the PR from Draft status to "Ready for Review". |
…ror on plugin install fail, and rethrow any other error
|
OK, the basic idea of this PR is ready. I confirmed that it does solve the issue and achieve the goal of letting plugin installs (and the rest of blueprint steps) continue even if one fails. To explain the proposed solution: in the function In the Playground website, when blueprint steps are compiled from the URL query API, it defines this error callback (here). For now, if the error happens in the I tried to implement this in a way that maintains current behavior as much as possible, except for where it's needed in the Query API, so that errors are thrown as expected for any other use of Blueprints (such as via Playground Client module, or on server side). I also tried to make it generally useful, in case there are other kinds of steps that allowed to fail without stopping the entire process. If you agree with this approach, I can add a test for the |
…Completed for success/error handling per blueprint
… installs to continue even if any fails - as a placeholder until better error reporting is ready
dmsnell
left a comment
There was a problem hiding this comment.
love how this is shaping up. makes me want to see if we can push forward on the toast component
|
@eliot-akira I'd love to have a mechanism of handling errors (and output) baked into Blueprints and it's becoming increasingly relevant for the extenders, too. Would you be open to doing a few more iterations here? Or, if you don't have the bandwidth at the time, would you mind if I pushed some commits this PR? |
@adamziel Sure, please do. To be honest I don't think I have the bandwidth to carry this through to the finish line in a timely manner. There are some unresolved questions about the specifics of how the feature works internally, and also the public interface like the naming of options/methods, and how users are expected to use this feature. It probably will require making detailed decisions toward a clear concept/design. @dmsnell Sorry I dropped the ball on this one, I got swept away by other concerns and haven't had the time or mental space to discuss and solve this completely. Looks like I bit off more than I could chew! |
|
OK, I refactored the changes in this PR into a new option I haven't tested yet if this works as expected, in particular:
Also, a question remains if this interface is designed in a way that can support the implementation of optional or fail-able steps, such as a step option that indicates whether the step is required or not for the rest of the blueprint. So the public interface is still a draft in progress, to see how best to solve these related issues. |
| /** | ||
| * Convert a Promise into a boolean indicating if it resolved or rejeted. | ||
| */ | ||
| const didResolve = <P extends Promise<any>>(promise: P): Promise<boolean> => |
There was a problem hiding this comment.
didResolve() === false suggests the promise may still be pending. I wonder what would be a more descriptive name here.
There was a problem hiding this comment.
It was tough to find a suitable naming for this didResolve function. I consulted MDN: Promise to see what the standard terminology is.
A Promise is in one of these states:
- pending: initial state, neither fulfilled nor rejected.
- fulfilled: meaning that the operation was completed successfully.
- rejected: meaning that the operation failed.
A promise is said to be settled if it is either fulfilled or rejected, but not pending.
In the end I settled on a verbose name isSettledPromiseResolved, to make it clear that it's checking an already settled promise for its resolved status.
| try { | ||
| const result = await run(playground); | ||
| stepPromise?.resolve(result); | ||
| onStepCompleted(result, step); |
There was a problem hiding this comment.
If the onStepCompleted handler throws an error, the stepPromise?.reject(error); will run. It would be nice to just rethrow that error so the API consumer may notice and correct it.
There was a problem hiding this comment.
Instead of rethrowing, the two lines stepStatus?.resolve(result) and onStepCompleted have been moved below the try/catch clause. If the latter throws (by the user or an unexpected error), it bubbles up to the API consumer, the caller of CompiledBlueprint.run().
| stepPromise?.resolve(result); | ||
| onStepCompleted(result, step); | ||
| } catch (error) { | ||
| stepPromise?.reject(error); |
There was a problem hiding this comment.
I wonder whether the failure consequences should be possible to specify per step type, e.g. rm could default to triggering a warning but not stopping the execution, while defineWpConfigConsts would default to throwing an error and stopping the execution.
There seem to be a hierarchy of control here:
- Consumer has a preference. If missing, fall back to...
- Step has a preference. If missing, fall back to...
- Blueprint engine has a preference – that's implicit from the code structure here
It would be nice to stop the execution as soon as we know the failure is fatal. Imagine the first step of a work-intense Blueprint fails and we already know this is irrecoverable – stopping right then and there would spare the user 30s of waiting before learning about the error.
Code-wise, it could mean a logic similar to this:
| stepPromise?.reject(error); | |
| stepPromise?.reject(error); | |
| if( isFailureFatal( step, error ) ) { | |
| throw e; | |
| } |
Now, the isFailureFatal would need to know about both the step preference and the user preference, which means we need to provide developers with a way of overriding the default behavior on a granular, per-step basis.
This PR introduces a decision point in the onStepsCompleted callback registered by the application calling the compileBlueprint function. I think it would be more useful to make that decision inside of a Blueprint. Imagine a live product demo. The installPlugin installing the demoed plugin is indispensable, but the next one installing a dev utility plugin can likely be skipped if it fails.
Technically, this could mean introducing a new step-level property:
export type StepDefinition = Step & {
progress?: {
weight?: number;
caption?: string;
};
+ onFailure: 'abort' | 'skip';
};This would enable writing blueprints such as...
{
"steps": [
{
"step": "installPlugin",
"pluginZipFile": {
"resource": "wordpress.org/plugins",
"slug": "my-product"
},
"onFailure": "abort"
},
{
"step": "installPlugin",
"pluginZipFile": {
"resource": "wordpress.org/plugins",
"slug": "devtools"
},
"onFailure": "skip"
}
}The default value could be abort. Each step handler would be able to override it, for example like this:
/**
* Removes a file at the specified path.
*/
export const rm: StepHandler<RmStep> = async (playground, { path }) => {
await playground.unlink(path);
};
import { defaultStepFailureHandlers } from '.';
defaultStepFailureHandlers.set( 'rm', 'continue' );The compileStep() function could then output the desired failure behavior:
function compileStep<S extends StepDefinition>(
step: S,
{
semaphore,
rootProgressTracker,
totalProgressWeight,
}: CompileStepArgsOptions
): { run: CompiledStep; step: S; resources: Array<Resource> } {
return {
run,
step,
resources,
onFailure: 'onFailure' in step ? step.onFailure : defaultStepFailureHandlers.get( step.step ) ?? 'abort'
};And this loop could handle it as follows:
for (const { run, step, onFailure } of compiled) {
const stepPromise = progressPromises.get(step);
try {
const result = await run();
onStepSettled(step, SUCCESS, result);
} catch(e) {
onStepSettled(step, FAILURE, e);
if( onFailure === 'abort' ) {
throw e;
}
}There was a problem hiding this comment.
I wonder how the ideas above resonate with you @eliot-akira @dmsnell @bgrgicak
There was a problem hiding this comment.
I think it fits within the idea I had, which was to expose each to the application and let it decide.
we could provide default behaviors, which I think should be rare, and then if someone in userspace adds their own handler for the step, then they could take their own action.
or, we could expose our default handling to the callback and only if they implement the callback would we use theirs instead.
There was a problem hiding this comment.
I like this approach, it would at least help with development and while creating blueprints.
Personally, I love opinionated code that I can adjust, so I would go with having defaults for each step, but let users override them. Maybe even include a global override for all steps.
There was a problem hiding this comment.
Thank you for the detailed review and feedback. I'm starting to get a feel for the public interface that's shaping up.
I like the idea of adding to the StepDefinition a new property onFailure, with value abort or skip. That addresses the original issue that motivated this PR, which is to allow some plugins to be skipped if install fails, while still keeping plugins required by default (error on install fail).
It makes sense for each step handler to have a default failure mode (abort or skip), which can be overridden by the user.
I'll get started on implementing the suggestions, and will either resolve each reviewed point or reply with any questions.
There was a problem hiding this comment.
Thank you so much @eliot-akira, you are the best!
Ensures the Blueprint steps throw an error on failure. For now, the error is merely reported in the console. In the future, there may be a nice UI for this. This PR is an addition to #605 as it prepares the Blueprint steps for the changes in the Blueprint compilation engine. Detecting a runPHP failure required adjusting the php_wasm.c code to return the correct exit code. The previous method of inferring the exit code was replaced by simply returning `EG(exit_status)` which is populated by the zend engine. Furthermore, this PR ships additional unit tests for some Blueprint steps to ensure they indeed throw as expected. ## Changes description ### `throwOnError` option `BasePHP.run()` now accepts a `throwOnError` option that throws an error whenever PHP returns an exit code different than `0`: ```js // This will throw a JavaScript exception await php.run({ throwOnError: true, code: '<?php no_such_function()' }); ``` This happens in the following cases: * syntax error * undefined function call * fatal error * calling `exit(1)` * uncaught PHP exception * ...probably some more This option is set to `true` by all the Blueprint steps. ## Remaining work - [x] Rebuild all the PHP.wasm versions ## Testing instructions This PR comes with test coverage so confirm all the tests pass in CI. Also, apply this PR locally and visit the following URL: http://localhost:5400/website-server/#{%22steps%22:[{%22step%22:%22runPHP%22,%20%22code%22:%22%3C?php%20no_such_fn();%22}]} It should reveal useful error information in the console.
…d a way for each step type to define default fail mode
…nd `shouldBoot`
…error - the default shouldBoot() will consider the step completed, so the site can continue to boot - Provide optional `onStepError` callback so website can display error message on plugin install fail
|
OK, I implemented the suggestions with my own take on some of the details. I've added a blueprint step option called It meets these requirements, which I wrote down before starting this round of changes.
The compileBlueprint option I also added a compileBlueprint option The callback can optionally throw an error to abort the rest of the steps (same as It's going in the right direction I think, but would appreciate another review. For manual testing:
|
| // TODO: Open modal to display error message | ||
| alert( | ||
| `Failed to install: ${ | ||
| (step.pluginZipFile as CorePluginReference).slug || |
There was a problem hiding this comment.
This type coersion to CorePluginReference is questionable. It was a temporary workaround because step.pluginZipFile is a FileReference, a union of several types which have property slug, name, path, or nothing for the name.
| for (const { step } of compiled) { | ||
| completedStatus.set( | ||
| step, | ||
| await isSettledPromiseResolved( |
There was a problem hiding this comment.
Will this wait forever if the finally section is triggered by an error thrown before all the steps are run?
| // Either someone made a choice to boot or all steps succeeded. | ||
| const boot = shouldBoot | ||
| ? shouldBoot(completedStatus) | ||
| : await isSettledPromiseResolved( |
There was a problem hiding this comment.
At this point, no further steps are executed. Also, nothing seems to await stepPromises – do we need to reason about status as promises, then? Or could the status information be stored in a synchronous data structure like Map<StepDefinition, { didRun, error, result }>?
|
There's some really good ideas in this PR. Since Blueprints are transitioning into a PHP library, let's freeze the set of features offered by the TypeScript implementation and keep exploring those ideas in the new, dedicated repository: https://github.com/WordPress/blueprints/. I'll go ahead and close this PR. Thank you so much for your work @eliot-akira! |
I was wondering how that effort related to this PR. That makes sense to focus on the new implementation. I'll follow along with the progress there. To be honest I was struggling a bit with this PR, and still wasn't totally clear on how the async/promises logic was supposed to work. Off topic, but recently I had a somewhat similar situation - start many concurrent tasks (convert incoming text to speech) while handling their results sequentially (play the generated audio) - and I realized I need more practice to be fluent with async stuff. In that case, I discovered what would really help solve it was
It probably would have helped simplify some of the logic in this PR, but it's not fully supported by JavaScript runtimes yet (notably Safari/WebKit and Node.js). |
What is this PR doing?
Add a callback
onBlueprintStepErrorto gather errors, so that plugin installs via website query parameter API can complete even if any fails. These errors should be displayed in a notification.What problem is it solving?
Issues #600 and #604, both caused due to entire blueprint steps getting stopped when a plugin fails to install.
How is the problem addressed?
TBD
Testing Instructions
TBD