Website query API: Continue plugin installs on error

[eliot-akira]

What is this PR doing?

Add a callback onBlueprintStepError to 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

[dmsnell]

so fast @eliot-akira! thanks - I might need a few days or more to review this.

[eliot-akira]

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".

[eliot-akira]

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 compileBlueprint, it adds an optional callback onStepError (here) which when defined will be called with any error that happens while running the blueprint step. The callback can decide what to do with the error, such as gathering them for notification, or rethrowing it to stop running the rest of steps.

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 installPlugin step, it just outputs to console error. For any other steps, it rethrows the error.

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 onStepError option.

[dmsnell]

love how this is shaping up. makes me want to see if we can push forward on the toast component

[adamziel]

@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?

[eliot-akira]

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!

[eliot-akira]

OK, I refactored the changes in this PR into a new option onStepProgress(), a callback that receives the step that's running and a promise in progress, which can be awaited or caught for error handling. I also reduced the number of types being added to a minimum, by inlining those that are short or used only once.

I haven't tested yet if this works as expected, in particular:

• If there is no onStepProgress callback passed, it should keep existing behavior by throwing an error for any failing step (such as plugin install), and stop the whole blueprint
• The onStepProgress callback should be able to:
  • Catch any error to let the rest of blueprint steps continue
  • Throw an error and stop the whole blueprint

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.

• "Login as editor" as an example for testing user roles blueprints#45
• Fix branch in theme test blueprints blueprints#40
• An invalid plugin slug should not cause following plugins to not be installed #600
• Make the import button work even if Playground fails to load #604

[adamziel]

didResolve() === false suggests the promise may still be pending. I wonder what would be a more descriptive name here.

[eliot-akira]

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.

[adamziel]

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.

[eliot-akira]

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().

[adamziel]

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:

1. Consumer has a preference. If missing, fall back to...
2. Step has a preference. If missing, fall back to...
3. 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:

Suggested change

	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;
						}
					}

[adamziel]

I wonder how the ideas above resonate with you @eliot-akira @dmsnell @bgrgicak

[dmsnell]

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.

[bgrgicak]

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.

[eliot-akira]

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.

[adamziel]

Thank you so much @eliot-akira, you are the best!

[eliot-akira]

OK, I implemented the suggestions with my own take on some of the details.

I've added a blueprint step option called failMode, with possible value abort or skip. The naming seemed more suitable compared to onFailure, onFail, or onError which sounds like a callback function.

It meets these requirements, which I wrote down before starting this round of changes.

• Skip mode: Catch any error to let the rest of blueprint steps continue
• Abort mode: Throw an error and stop the whole blueprint
  • Stop the execution as soon as we know the failure is fatal

---

The compileBlueprint option onStepsCompleted has been renamed to shouldBoot, which takes a map of step completion status, and returns a boolean to decide if the site should continue booting.

I also added a compileBlueprint option onStepError, to let the website have a chance to display an error message in a modal - currently using alert().

The callback can optionally throw an error to abort the rest of the steps (same as abort mode), or let them continue (same as skip mode). This part I'm a bit unsure of the logic, how it interacts with the fail mode.

---

It's going in the right direction I think, but would appreciate another review.

For manual testing:

• This URL should show an error message, then continue booting the site: http://localhost:5400/website-server/?plugin=not-existing

[eliot-akira]

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.

[adamziel]

Will this wait forever if the finally section is triggered by an error thrown before all the steps are run?

[adamziel]

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 }>?

[adamziel]

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!

[eliot-akira]

Blueprints are transitioning into a PHP library, let's freeze the set of features offered by the TypeScript implementation

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 Promise.withResolvers().

The key difference when using Promise.withResolvers() is that the resolution and rejection functions now live in the same scope as the promise itself, instead of being created and used once within the executor.

This may enable some more advanced use cases, such as when reusing them for recurring events, particularly with streams and queues. This also generally results in less nesting than wrapping a lot of logic within the executor.

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).