Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support installing WP as needed in Playground remote #1841

Open
wants to merge 7 commits into
base: trunk
Choose a base branch
from
Open

Support installing WP as needed in Playground remote #1841

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9ebeb51
Support installing WP as needed in Playground remote
brandonpayton Oct 4, 2024
2f53431
Fix vite dev build for website importing from remote package
brandonpayton Oct 8, 2024
ccde6fe
Export reusable looks-like-playground-dir function
brandonpayton Oct 8, 2024
5d555f9
Use universal looks-like-playground function
brandonpayton Oct 8, 2024
51dd315
Fix type error
brandonpayton Oct 8, 2024
3b1cffd
Tweak location of shared function
brandonpayton Oct 8, 2024
3b56699
Fix whitespace typo
brandonpayton Oct 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion packages/playground/client/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,11 @@ export interface StartPlaygroundOptions {
*/
onBeforeBlueprint?: () => Promise<void>;
mounts?: Array<MountDescriptor>;
shouldInstallWordPress?: boolean;
/**
* 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';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this?

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ashfame which of these reads clearer to you?

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@adamziel adamziel Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/**
* The string prefix used in the site URL served by the currently
* running remote.html. E.g. for a prefix like `/scope:playground/`,
Expand Down
31 changes: 29 additions & 2 deletions packages/playground/remote/src/lib/worker-thread.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,10 @@ import {
sqliteDatabaseIntegrationModuleDetails,
MinifiedWordPressVersionsList,
} from '@wp-playground/wordpress-builds';
import { directoryHandleFromMountDevice } from '@wp-playground/storage';
import {
directoryHandleFromMountDevice,
fileExistsUnderDirectoryHandle,
} from '@wp-playground/storage';
import { randomString } from '@php-wasm/util';
import {
spawnHandlerFactory,
Expand Down Expand Up @@ -45,6 +48,7 @@ import {
bootWordPress,
getFileNotFoundActionForWordPress,
getLoadedWordPressVersion,
looksLikePlaygroundDirectory,
} from '@wp-playground/wordpress';
import { wpVersionToStaticAssetsDirectory } from '@wp-playground/wordpress-builds';
import { logger } from '@php-wasm/logger';
Expand Down Expand Up @@ -72,7 +76,7 @@ export type WorkerBootOptions = {
scope: string;
withNetworking: boolean;
mounts?: Array<MountDescriptor>;
shouldInstallWordPress?: boolean;
shouldInstallWordPress?: boolean | 'auto';
};

/** @inheritDoc PHPClient */
Expand Down Expand Up @@ -188,6 +192,29 @@ export class PlaygroundWorkerEndpoint extends PHPWorker {
}

try {
if (shouldInstallWordPress === 'auto') {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How could we make this reusable for Playground CLI?

Copy link
Member Author

@brandonpayton brandonpayton Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@adamziel adamziel Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Default to installing WordPress unless we detect
// it in one of the mounts.
shouldInstallWordPress = true;

// NOTE: This check is insufficient if a complete WordPress
// installation is composed of multiple mounts.
for (const mount of mounts) {
const dirHandle = await directoryHandleFromMountDevice(
mount.device
);
const fileExistsUnderMount = (relativePath: string) =>
fileExistsUnderDirectoryHandle(dirHandle, relativePath);

if (
await looksLikePlaygroundDirectory(fileExistsUnderMount)
) {
shouldInstallWordPress = false;
break;
}
}
}

// Start downloading WordPress if needed
let wordPressRequest = null;
if (shouldInstallWordPress) {
Expand Down
25 changes: 25 additions & 0 deletions packages/playground/storage/src/lib/browser-fs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,31 @@ export async function opfsPathToDirectoryHandle(
return handle;
}

export async function fileExistsUnderDirectoryHandle(
directoryHandle: FileSystemDirectoryHandle,
relativePath: string
): Promise<boolean> {
const parts = relativePath.split('/').filter((p) => p.length > 0);
const fileName = parts.pop();
if (fileName === undefined) {
throw new Error('Invalid relative path.');
}

for (const part of parts) {
try {
directoryHandle = await directoryHandle.getDirectoryHandle(part);
} catch (e: any) {
return false;
}
}
try {
await directoryHandle.getFileHandle(fileName);
return true;
} catch (e) {
return false;
}
}

export async function directoryHandleToOpfsPath(
directoryHandle: FileSystemDirectoryHandle
): Promise<string> {
Expand Down
57 changes: 11 additions & 46 deletions packages/playground/website/src/lib/state/redux/boot-site-client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import { logger } from '@php-wasm/logger';
import { setupPostMessageRelay } from '@php-wasm/web';
import { startPlaygroundWeb } from '@wp-playground/client';
import { PlaygroundClient } from '@wp-playground/remote';
import { fileExistsUnderDirectoryHandle } from '@wp-playground/storage';
import { looksLikePlaygroundDirectory } from '@wp-playground/wordpress';
import { getRemoteUrl } from '../../config';
import { setActiveModal, setActiveSiteError } from './slice-ui';
import { PlaygroundDispatch, PlaygroundReduxState } from './store';
Expand Down Expand Up @@ -72,8 +74,15 @@ export function bootSiteClient(
let isWordPressInstalled = false;
if (mountDescriptor) {
try {
isWordPressInstalled = await playgroundAvailableInOpfs(
await directoryHandleFromMountDevice(mountDescriptor.device)
const mountDirHandle = await directoryHandleFromMountDevice(
mountDescriptor.device
);
isWordPressInstalled = await looksLikePlaygroundDirectory(
(relativePath: string) =>
fileExistsUnderDirectoryHandle(
mountDirHandle,
relativePath
)
);
} catch (e) {
logger.error(e);
Expand Down Expand Up @@ -207,47 +216,3 @@ export function bootSiteClient(
signal.onabort = null;
};
}

/**
* Check if the given directory handle directory is a Playground directory.
*
* @TODO: Create a shared package like @wp-playground/wordpress for such utilities
* and bring in the context detection logic from wp-now – only express it in terms of
* either abstract FS operations or isomorphic PHP FS operations.
* (we can't just use Node.js require('fs') in the browser, for example)
*
* @TODO: Reuse the "isWordPressInstalled" logic implemented in the boot protocol.
* Perhaps mount OPFS first, and only then check for the presence of the
* WordPress installation? Or, if not, perhaps implement a shared file access
* abstraction that can be used both with the PHP module and OPFS directory handles?
*
* @param dirHandle
*/
export async function playgroundAvailableInOpfs(
dirHandle: FileSystemDirectoryHandle
) {
// Run this loop just to trigger an exception if the directory handle is no good.
// eslint-disable-next-line @typescript-eslint/no-unused-vars
for await (const _ of dirHandle.keys()) {
break;
}

try {
/**
* Assume it's a Playground directory if these files exist:
* - wp-config.php
* - wp-content/database/.ht.sqlite
*/
await dirHandle.getFileHandle('wp-config.php', { create: false });
const wpContent = await dirHandle.getDirectoryHandle('wp-content', {
create: false,
});
const database = await wpContent.getDirectoryHandle('database', {
create: false,
});
await database.getFileHandle('.ht.sqlite', { create: false });
} catch (e) {
return false;
}
return true;
}
17 changes: 16 additions & 1 deletion packages/playground/wordpress/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,7 @@ export async function unzipWordPress(php: PHP, wpZip: File) {
: '/tmp/unzipped-wordpress';

// Dive one directory deeper if the zip root does not contain the sample
// config file. This is relevant when unzipping a zipped branch from the
// config file. This is relevant when unzipping a zipped branch from the
// https://github.com/WordPress/WordPress repository.
if (!php.fileExists(joinPaths(wpPath, 'wp-config-sample.php'))) {
// Still don't know the directory structure of the zip file.
Expand Down Expand Up @@ -384,6 +384,21 @@ export async function unzipWordPress(php: PHP, wpZip: File) {
}
}

/**
* 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);
}

function isCleanDirContainingSiteMetadata(path: string, php: PHP) {
const files = php.listFiles(path);
if (files.length === 0) {
Expand Down
Loading