Support installing WP as needed in Playground remote #1841

brandonpayton · 2024-10-04T03:59:41Z

Motivation for the change, related issues

Applications that integrate Playground do not have a good way of detecting whether a given Playground exists or has WordPress installed yet. Yet the @wp-playground/client's startPlaygroundWeb() function will attempt to install WP into a Playground by default and will cause a Playground boot failure if WP is already installed.

This PR expands startPlaygroundWeb()'s boolean shouldInstallWordPress argument to also support an "auto" value. When shouldInstallWordPress: "auto" is specified, the Playground boot process will now attempt to detect whether there is an existing WP install and only install WP if needed.

Related to:
WordPress/try-wordpress#44
WordPress/try-wordpress#62

cc @psrpinto @ashfame

Implementation details

This PR updates the worker thread to check each specified mount for the presence of expected WordPress+Playground files. If it finds the appearance of a Playground WP install, it skips downloading and installing WP, but if it finds nothing, it defaults to installing WP.

Testing Instructions (or ideally a Blueprint)

I ended up just testing this manually by:

Starting the dev server with npm run dev
Reloading the Playground web app with each possible value of shouldInstallWordPress hardcoded here:
- For false, loading temporary sites will fail because WordPress wasn't installed
- For true, loading temp sites works, but loading saved sites doesn't because Playground errors while trying to overwrite existing WP source files
- For auto, loading both temp sites and saved sites works.

brandonpayton · 2024-10-04T04:29:39Z

Getting this error when running npm run dev. Will need to look into it.

% npm run dev

> [email protected] dev
> nx dev playground-website


> nx run playground-website:dev

> nx run playground-remote:dev:development-for-website
  ➜  Local:   http://127.0.0.1:4400/
> nx run playground-website:"dev:standalone":development --hmr
  ➜  Local:   http://127.0.0.1:5400/website-server/
Failed to resolve import "virtual:remote-config" from "packages/playground/remote/src/lib/offline-mode-cache.ts". Does the file exist?
12:28:25 AM [vite] Internal server error: Failed to resolve import "virtual:remote-config" from "packages/playground/remote/src/lib/offline-mode-cache.ts". Does the file exist?
  Plugin: vite:import-analysis
  File: /Users/brandon/src/playground/packages/playground/remote/src/lib/offline-mode-cache.ts:3:29
  1  |  import { isURLScoped } from "@php-wasm/scopes";
  2  |  import { buildVersion } from "virtual:remote-config";
     |                                ^
  3  |  const CACHE_NAME_PREFIX = "playground-cache";
  4  |  const LATEST_CACHE_NAME = `${CACHE_NAME_PREFIX}-${buildVersion}`;
      at formatError (file:///Users/brandon/src/playground/node_modules/vite/dist/node/chunks/dep-bb8a8339.js:44062:46)
      at TransformContext.error (file:///Users/brandon/src/playground/node_modules/vite/dist/node/chunks/dep-bb8a8339.js:44058:19)
      at normalizeUrl (file:///Users/brandon/src/playground/node_modules/vite/dist/node/chunks/dep-bb8a8339.js:41844:33)
      at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
      at async file:///Users/brandon/src/playground/node_modules/vite/dist/node/chunks/dep-bb8a8339.js:41998:47
      at async Promise.all (index 1)
      at async TransformContext.transform (file:///Users/brandon/src/playground/node_modules/vite/dist/node/chunks/dep-bb8a8339.js:41914:13)
      at async Object.transform (file:///Users/brandon/src/playground/node_modules/vite/dist/node/chunks/dep-bb8a8339.js:44352:30)
      at async loadAndTransform (file:///Users/brandon/src/playground/node_modules/vite/dist/node/chunks/dep-bb8a8339.js:55026:29)
      at async viteTransformMiddleware (file:///Users/brandon/src/playground/node_modules/vite/dist/node/chunks/dep-bb8a8339.js:64430:32)

brandonpayton · 2024-10-04T05:07:11Z

If anyone wants to take this while I'm AFK today, please feel free. Otherwise, I hope to finish this next week.

brandonpayton · 2024-10-04T05:12:23Z

I'm guessing the vite dev server error is related to this

wordpress-playground/packages/playground/client/vite.config.ts

Lines 25 to 30 in 0a4cd5f

    
           // @wp-playground/client doesn't actually use the remote-config virtual 
        
           // module, @wp-playground/remote package does. @wp-playground/client imports 
        
           // a few things from @wp-playground/remote and, even though it doesn't 
        
           // involve the remote-config virtual module, the bundler still needs to know 
        
           // what to do when it sees `import from "virtual:remote-config"`. 
        
           buildVersionPlugin('remote-config'),

and the fact that this PR introduces a direct dependency between the website package on a real export from @wp-playground/remote. Maybe it would be fixed if the client package exports looksLikePlaygroundDirectory() from the @wp-playground/remote package and website imports from @wp-playground/client instead.

brandonpayton · 2024-10-04T05:25:47Z

Maybe it would be fixed if the client package exports looksLikePlaygroundDirectory() from the @wp-playground/remote package and website imports from @wp-playground/client instead.

This didn't help. But moving the looksLikePlaygroundDirectory() predicate to a different shared package like @wp-playground/storage avoids the error. That may not be the right location for the function. But knowing that the dev server works after that change is at least a notable result.

adamziel · 2024-10-07T18:07:08Z

I just dug up this old PR I made and I felt there's some connection with this work. I don't have anything specific, I mostly mean this as a potential source of inspiration. Maybe it's completely unrelated.

#1522

adamziel · 2024-10-07T21:48:45Z

packages/playground/remote/src/lib/worker-thread.ts

@@ -188,6 +189,24 @@ export class PlaygroundWorkerEndpoint extends PHPWorker {
 		}

 		try {
+			if (shouldInstallWordPress === 'auto') {


How could we make this reusable for Playground CLI?

How could we make this reusable for Playground CLI?

Both Playground remote and CLI leverage bootWordPress(), but AFAICT, everything before that is custom setup for their individual context. I'll consider whether there is more we can share, but at the very least, we could implement the same logic for CLI.

The CLI's skipWordPressSetup argument is a potential conflict and maybe could be deprecated in favor of a shouldInstallWordPress argument. Or they could be complementary.

Thanks for drawing my attention to this.

How could we make this reusable for Playground CLI?

Both Playground remote and CLI leverage bootWordPress(), but AFAICT, everything before that is custom setup for their individual context. I'll consider whether there is more we can share, but at the very least, we could implement the same logic for CLI.

I was thinking of saying the following:

To be able to implement this check in a way that works for both PHP and CLI, maybe such a feature should be moved into bootWordPress(). That way, we can write the check in PHP and...

But if we want to allow parallel downloads of PHP and WP resources in a web context, we need to check before PHP has been downloaded and loaded.

I made a reusable looksLikePlaygroundDirectory() function under @wp-playground/wordpress. The relative nature of the fileExists() predicate feels a little clunky, but I think this is workable for sharing the looks-like-playground-dir heuristic.

/** * Check if the given directory handle directory is a Playground directory. * * @param fileExists Function A function that checks if a file exists relative to an assumed directory. * @returns Promise<boolean> Whether the directory looks like a Playground directory. */ export async function looksLikePlaygroundDirectory( fileExists: (relativePath: string) => Promise<boolean> ) { const results = await Promise.all( ['wp-config.php', 'wp-content/database/.ht.sqlite'].map(fileExists) ); return results.every(Boolean); }

There's something similar in wp-now, you may want to review their "WordPress mode". One assumption this implementation makes is that we're running on SQLite whereas Playground CLI supports MySQL, too

Having support for this could improve the Playground tester's performance.
Every test downloads WordPress and it's the majority of the test time. Being able to download WP once and mount it would be helpful.

brandonpayton · 2024-10-08T03:58:40Z

Getting this error when running npm run dev. Will need to look into it.

Note: The vite dev server error went away when pivoting away from @wp-playground/website importing from @wp-playground/remote.

brandonpayton · 2024-10-08T04:04:19Z

I think this is ready for review.

Question:
Should the value be "auto", "auto-detect", "detect", or something else?

adamziel · 2024-10-08T07:39:50Z

I think this is ready for review.

Question:

Should the value be "auto", "auto-detect", "detect", or something else?

"if-missing"?

ashfame · 2024-10-11T12:53:20Z

It would be nice to know whether WP was just installed or we are just "resuming", so that diff blueprint steps can be run accordingly.

adamziel · 2024-10-11T15:41:38Z

@ashfame oh, interesting. You're asking about the same problems we've solved for in the website app. See bootSiteClient() here:

wordpress-playground/packages/playground/website/src/lib/state/redux/boot-site-client.ts

Lines 97 to 113 in 6295aa2

    
           let blueprint: Blueprint; 
        
           if (isWordPressInstalled) { 
        
           	blueprint = site.metadata.runtimeConfiguration; 
        
           } else { 
        
           	blueprint = site.metadata.originalBlueprint; 
        
           	// Log the names of provided Blueprint's steps. 
        
           	// Only the names (e.g. "runPhp" or "login") are logged. Step options like 
        
           	// code, password, URLs are never sent anywhere. 
        
           	const steps = (blueprint?.steps || []) 
        
           		?.filter( 
        
           			(step: any) => !!(typeof step === 'object' && step?.step) 
        
           		) 
        
           		.map((step) => (step as StepDefinition).step); 
        
           	for (const step of steps) { 
        
           		logTrackingEvent('step', { step }); 
        
           	} 
        
           }

For a moment I thought about exporting this entire function as a public API, but it's very website-specific after all. The conditional Blueprint execution, though, would be another useful addition to startPlaygroundWeb(). Perhaps something like...

startPlaygroundWeb({
	blueprint,
	shouldRunBlueprint: 'when-installing-wordpress' | 'always' // naming tbd
});

...with the default being when-installing-wordpress.

adamziel · 2024-10-14T21:33:42Z

packages/playground/client/src/index.ts

+	 * Whether to install WordPress. Value may be boolean or 'auto'.
+	 * If 'auto', WordPress will be installed if it is not already installed.
+	 */
+	shouldInstallWordPress?: boolean | 'auto';


How about this?

Suggested change

shouldInstallWordPress?: boolean | 'auto';

shouldInstallWordPress?: boolean | 'if-not-installed';

I like if-not-installed better than auto but is it really necessary to define a string default here? Can't we do a check against undefined to infer "auto" behavior & prevent mixing of type?

@ashfame which of these reads clearer to you?

const config = { shouldInstallWordPress: auto ? undefined : false; }; // or const config = { shouldInstallWordPress: auto ? 'if-not-installed' : false; };

Not sure I follow. What I meant was keeping shouldInstallWordPress?: boolean as it is. When that property is explicitly specified, do what's being asked (Current behavior) and when not defined at all, just assume the "auto" behavior i.e. install if no WP files are found, otherwise let it be.

Sure, that makes sense and sounds convenient for omitting the shouldInstallWordPress option:

startPlaygroundWeb({ // shouldInstallWordPress is not here, which means we use the "auto" mode });

I was just pointing out that it reads awkward when you want to explicitly declare the shouldInstallWordPress key and need to explicitly type in undefined as a value:

startPlaygroundWeb({ // This is not very informative: shouldInstallWordPress: undefined });

Yep, a user should never have to explicitly pass undefined. But I don't see why someone would have to do that with my suggestion? IF they need to specify it, they should know whether it's a true or false. And if they don't they should omit and let the default behavior take place, no?

It's not uncommon to define object literals like this:

startPlaygroundWeb({ shouldInstallWordPress: getUserPreference() });

or like this:

startPlaygroundWeb({ shouldInstallWordPress });

or like this:

startPlaygroundWeb({ shouldInstallWordPress: condition ? true : undefined });

In which case the undefined literal would show up somewhere.

Sure! Implementation details aside, I think we are on the same page as to how it should behave, which is: Do as explicitly asked and figure out if we need to, as the default behavior. Thought I will reiterate it, in case this PR was waiting on me to close the loop of our conversation. cc: @brandonpayton

ashfame · 2024-10-15T20:42:56Z

shouldRunBlueprint: 'when-installing-wordpress' | 'always' // naming tbd

@adamziel This allows conveying, whether "always" or just on "first boot". Doesn't let a user specify blueprint for only post-installation boots. In simple terms, the user would want to specify two different blueprints - one for first boot and another for second boot onwards, right? Former never runs after running once and latter has no context to run prior or along with former.

Also, I would like to avoid property coupling here. And since current behavior is to run the blueprint always with each boot. I think the safest (and also easiest/quickest) way would be to add another blueprint which is meant to be executed always, just not on first boot.

Naming is hard, but something like trailingBlueprint or nextBlueprint.

This way, the intent its clear. Backwards compatibility is maintained and properties are not coupled. cc: @brandonpayton

adamziel · 2024-10-15T22:18:48Z

This seems to touch a deeper problem of distinguishing between the Blueprint steps (install these plugins) and the runtime configuration (use this PHP version). I'd love to separate these as we harmonize the boot across the webapp and the CLI. Then we could have a runtimeConfig option and a separate blueprint parameter.

This still wouldn't solve for first-time Blueprints and subsequent Blueprints, though. Perhaps Blueprint could also be a callback, e.g.

startPlaygroundWeb({
	// This works:
	blueprint: { "plugins": ["hello-dolly"] }
});

startPlaygroundWeb({
	// This could also work:
	blueprint: ({ isWordPressInstalled }) => {
		return isWordPressInstalled ? initialBlueprint : subsequentBlueprint;
	}
});

But actually I wanted to ask – what problem are we solving here with the initial/subsequent Blueprint execution? Is it about try-wordpress only running the Blueprint once? Or are you having some ideas about applying new Blueprints to the same site later on?

ashfame · 2024-10-16T16:20:14Z

@adamziel While liberating a site, we would only realize the current site is an e-commerce store, post initial setup of playground as user express interest in liberating "products". At this point, we desire to run a bunch of steps (imagine installing woo commerce + some add-ons + config), which is why we would need that ability to specify yet another blueprint, preferably without even reloading the playground.

So, I guess this requires both: decoupling running of blueprints from boot (ability to arbitrary invoke a blueprint) and running subsequent blueprints and not being limited to just the initial.

I am not sure the overlap with what you call runtimeConfig as yo mention it, so I defer to you for that.

But then, should we call it something else other than blueprint?

I don't think so. Here is how I think about it:

initial blueprint for setup: laying foundation + ground floor (probably what most would need)
and then later, another blueprint for first floor :D and so on

brandonpayton · 2024-11-01T23:16:07Z

@adamziel @bgrgicak I just looked at this again and am wondering:

Is there a reason we shouldn't just have Playground web client always auto-install WordPress if needed? Is there a case where we don't want WP to be installed even if it isn't present? If we're worried about making the wrong choice on behalf of users, we could look at strengthening our is-WP-installed checks.

brandonpayton · 2024-11-01T23:19:10Z

@adamziel While liberating a site, we would only realize the current site is an e-commerce store, post initial setup of playground as user express interest in liberating "products". At this point, we desire to run a bunch of steps (imagine installing woo commerce + some add-ons + config), which is why we would need that ability to specify yet another blueprint, preferably without even reloading the playground.

@ashfame what if Playground client exposed a runBlueprintSteps() method or something similar? It could take a set of Blueprint steps and run them under the runtime configuration of the booted Playground.

@adamziel @bgrgicak what do you think of that possibility for a separate PR?

bgrgicak · 2024-11-04T08:17:04Z

what if Playground client exposed a runBlueprintSteps() method or something similar? It could take a set of Blueprint steps and run them under the runtime configuration of the booted Playground.

I like this. Technically allowing JS API users to run Blueprint steps is similar to running Blueprints on existing sites, so this would be a good way for us to start that exploration.

ashfame · 2024-11-04T17:07:44Z

@brandonpayton Yep, being able to invoke a runBlueprintSteps() fits the bill 💯

adamziel · 2024-11-04T20:34:39Z

Sounds good to me. I'd just be careful with the client bundle size, bundling @wp-playground/blueprints might (or might not!) add a lot of data and might be best loaded asynchronously. Should be easy enough, though.

brandonpayton · 2024-11-05T02:01:54Z

Sounds good to me. I'd just be careful with the client bundle size, bundling @wp-playground/blueprints might (or might not!) add a lot of data and might be best loaded asynchronously. Should be easy enough, though.

Good point! Thanks for word of the caution.

brandonpayton · 2024-11-05T04:56:26Z

Note:
I started looking at adding runBlueprintSteps(). The actual implementation should be straightforward and defined within startPlaygroundWeb() because that is where the original blueprint is applied and the only place the knowledge of the original Blueprint is present.

The awkward part to me is the typing. The @wp-playground/client package's startPlaygroundWeb() function returns a PlaygroundClient type which was declared by the @wp-playground/remote package. We need to add a runBlueprintSteps() method in @wp-playground/client to a type which is declared in the dependency @wp-playground/remote.

I'm thinking about renaming the type export of @wp-playground/remote to be something like PlaygroundWorkerClient and then declaring a PlaygroundClient type within the @wp-playground/client package that includes the new method.

brandonpayton added [Type] Bug An existing feature does not function as intended [Package][@wp-playground] Client labels Oct 4, 2024

brandonpayton requested a review from a team October 4, 2024 03:59

brandonpayton self-assigned this Oct 4, 2024

adamziel force-pushed the trunk branch 3 times, most recently from 680cd19 to 2e376d2 Compare October 4, 2024 09:24

Support installing WP as needed in Playground remote

9ebeb51

brandonpayton force-pushed the support-auto-detect-of-wp-install branch from 0a4cd5f to 9ebeb51 Compare October 7, 2024 21:16

adamziel reviewed Oct 7, 2024

View reviewed changes

brandonpayton added 6 commits October 7, 2024 22:21

Fix vite dev build for website importing from remote package

2f53431

Export reusable looks-like-playground-dir function

ccde6fe

Use universal looks-like-playground function

5d555f9

Fix type error

51dd315

Tweak location of shared function

3b1cffd

Fix whitespace typo

3b56699

brandonpayton marked this pull request as ready for review October 8, 2024 04:02

adamziel reviewed Oct 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support installing WP as needed in Playground remote #1841

Support installing WP as needed in Playground remote #1841

brandonpayton commented Oct 4, 2024 •

edited

Loading

brandonpayton commented Oct 4, 2024

brandonpayton commented Oct 4, 2024

brandonpayton commented Oct 4, 2024

brandonpayton commented Oct 4, 2024

adamziel commented Oct 7, 2024 •

edited

Loading

adamziel Oct 7, 2024

brandonpayton Oct 7, 2024 •

edited

Loading

brandonpayton Oct 7, 2024

brandonpayton Oct 8, 2024

adamziel Oct 8, 2024 •

edited

Loading

bgrgicak Nov 4, 2024

brandonpayton commented Oct 8, 2024

brandonpayton commented Oct 8, 2024

adamziel commented Oct 8, 2024

ashfame commented Oct 11, 2024

adamziel commented Oct 11, 2024 •

edited

Loading

adamziel Oct 14, 2024

ashfame Oct 15, 2024

adamziel Oct 15, 2024

ashfame Oct 15, 2024

adamziel Oct 15, 2024 •

edited

Loading

ashfame Oct 16, 2024

adamziel Oct 16, 2024

ashfame Nov 4, 2024

ashfame commented Oct 15, 2024

adamziel commented Oct 15, 2024

ashfame commented Oct 16, 2024

brandonpayton commented Nov 1, 2024

brandonpayton commented Nov 1, 2024

bgrgicak commented Nov 4, 2024

ashfame commented Nov 4, 2024

adamziel commented Nov 4, 2024 •

edited

Loading

brandonpayton commented Nov 5, 2024

brandonpayton commented Nov 5, 2024

	shouldInstallWordPress?: boolean \| 'auto';
	shouldInstallWordPress?: boolean \| 'if-not-installed';

Support installing WP as needed in Playground remote #1841

Are you sure you want to change the base?

Support installing WP as needed in Playground remote #1841

Conversation

brandonpayton commented Oct 4, 2024 • edited Loading

Motivation for the change, related issues

Implementation details

Testing Instructions (or ideally a Blueprint)

brandonpayton commented Oct 4, 2024

brandonpayton commented Oct 4, 2024

brandonpayton commented Oct 4, 2024

brandonpayton commented Oct 4, 2024

adamziel commented Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

brandonpayton Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamziel Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonpayton commented Oct 8, 2024

brandonpayton commented Oct 8, 2024

adamziel commented Oct 8, 2024

ashfame commented Oct 11, 2024

adamziel commented Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamziel Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashfame commented Oct 15, 2024

adamziel commented Oct 15, 2024

ashfame commented Oct 16, 2024

brandonpayton commented Nov 1, 2024

brandonpayton commented Nov 1, 2024

bgrgicak commented Nov 4, 2024

ashfame commented Nov 4, 2024

adamziel commented Nov 4, 2024 • edited Loading

brandonpayton commented Nov 5, 2024

brandonpayton commented Nov 5, 2024

brandonpayton commented Oct 4, 2024 •

edited

Loading

adamziel commented Oct 7, 2024 •

edited

Loading

brandonpayton Oct 7, 2024 •

edited

Loading

adamziel Oct 8, 2024 •

edited

Loading

adamziel commented Oct 11, 2024 •

edited

Loading

adamziel Oct 15, 2024 •

edited

Loading

adamziel commented Nov 4, 2024 •

edited

Loading