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

Jump to bottom

changes to use wordpress-playground for teaching #1145

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

changes to use wordpress-playground for teaching #1145

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
847fcc3
re-branched from trunk
rhildred Mar 26, 2024
b3d0e61
make branch with minimum changes for a pull request
rhildred Mar 26, 2024
67bcca1
Merge branch 'WordPress:trunk' into teaching
rhildred Mar 26, 2024
7e84060
Merge branch 'WordPress:trunk' into teaching
rhildred Apr 3, 2024
2d076ef
add sitemap.*.xml$ to the things that generate a .php request.
richard-hildred Apr 3, 2024
05fe606
Merge branch 'WordPress:trunk' into teaching
rhildred Apr 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,10 @@
"scripts": {
"build": "nx run-many --all --target=build",
"build:website": "nx build playground-website",
"build:remote": "nx build playground-remote",
"build:docs": "nx build docs-site",
"deploy:docs": "gh-pages -d dist/docs/build -t true",
"dev": "nx dev playground-website",
"dev": "nx dev playground-remote --host 0.0.0.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

How would we develop the website version in this case?

Copy link
Author

@rhildred rhildred Apr 4, 2024 •
edited

Choose a reason for hiding this comment

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

That's a great question, and gets right to the heart of the matter. I needed remote.html to be served from my origin so that I could access the origin private file system. It wasn't clear to me how the build: website playground.wordpress.net worked at all. I could run npm run dev and it worked but I couldn't figure out how you got the index in to the production build. Adding my own index to the remote target let me deploy on cloudflare or even gh pages with a "normal" build.

Would it be acceptable to add a custom element to the client package? One would install the client package with npm and consume the custom element in a basic vite index.html.

I would love to speak about this @bgrgicak . You are ahead of me in time and I have students now for the rest of the week. If we could speak next week, it would be amazing. Monday and Tuesday mornings EDT (Toronto time) are the best for me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Aha, OPFS! I think you can only transfer OPFS handles between the same site, which typically also means the same origin. I remember failing to build an app that gets an OPFS handle on another domain and then gives playground.wordpress.net access to that handle.

I wonder what would happen if remote.html was responsible for getting the OPFS handle – in theory it would all stay within the same domain so perhaps that would be a way to enable custom Playground apps that are still able to use OPFS.

Would it be acceptable to add a custom element to the client package? One would install the client package with npm and consume the custom element in a basic vite index.html.

Potentially, yes! I'm just not sure what do you mean by a custom element – would you elaborate?

Copy link
Collaborator

@bgrgicak bgrgicak Apr 5, 2024 •
edited

Choose a reason for hiding this comment

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

I would love to speak about this @bgrgicak

Please send me a message on the Making WordPress Slack (@bero) I'm happy to schedule some time to chat.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As a simple change, we could do this.

Suggested change
"dev": "nx dev playground-remote --host 0.0.0.0",
"dev": "nx dev playground-website",
"dev-remote": "nx dev playground-remote --host 0.0.0.0",

"format": "nx format",
"format:uncommitted": "nx format --fix --parallel --uncommitted",
"lint": "nx run-many --all --target=lint",
Expand Down
2 changes: 1 addition & 1 deletion packages/php-wasm/node/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
"types": "index.d.ts",
"gitHead": "2f8d8f3cea548fbd75111e8659a92f601cddc593",
"engines": {
"node": ">=18.18.2",
"node": ">=18.18.0",
"npm": ">=8.11.0"
}
}
6 changes: 5 additions & 1 deletion packages/php-wasm/universal/src/lib/php-request-handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,11 @@ export function seemsLikeAPHPRequestHandlerPath(path: string): boolean {
}

function seemsLikeAPHPFile(path: string) {
return path.endsWith('.php') || path.includes('.php/');
return (
path.endsWith('.php') ||
path.includes('.php/') ||
path.match(/sitemap.*.xml$/) != null
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this change needed? This could cause issues with requesting XML files as they could end up being run as PHP files instead of treated as static HTML.

);
}

function seemsLikeADirectoryRoot(path: string) {
Expand Down
17 changes: 1 addition & 16 deletions packages/playground/client/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ export async function startPlaygroundWeb({
onClientConnected = () => {},
sapiName,
}: StartPlaygroundOptions): Promise<PlaygroundClient> {
assertValidRemote(remoteUrl);
allowStorageAccessByUserActivation(iframe);

remoteUrl = setQueryParams(remoteUrl, {
Expand Down Expand Up @@ -171,22 +170,8 @@ async function doStartPlaygroundWeb(
return playground;
}

const officialRemoteOrigin = 'https://playground.wordpress.net';
function assertValidRemote(remoteHtmlUrl: string) {
const url = new URL(remoteHtmlUrl, officialRemoteOrigin);
if (
(url.origin === officialRemoteOrigin || url.hostname === 'localhost') &&
url.pathname !== '/remote.html'
) {
throw new Error(
`Invalid remote URL: ${url}. ` +
`Expected origin to be ${officialRemoteOrigin}/remote.html.`
);
}
}

function setQueryParams(url: string, params: Record<string, unknown>) {
const urlObject = new URL(url, officialRemoteOrigin);
const urlObject = new URL(url, window.location.href);
Copy link
Collaborator

@adamziel adamziel Apr 3, 2024 •
edited

Choose a reason for hiding this comment

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

That check is in place to offer a clear error message in case anyone confused https://playground.wordpress.net/ for https://playground.wordpress.net/remote.html. Did it break something for you?

Copy link
Author

Choose a reason for hiding this comment

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

Yes it broke me, in that I need to have remote.html with the same origin as my site so that I can use the origin private file system. The opfs integration is brilliant by the way. It is so much better than shared array buffer to integrate with. I would love a hint from the code on how you got that to work with the emscriptem fs by the way?????

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, that check should only kick in for playground.wordpress.net and localhost, not for custom deployments. Also, this new URL() should work with custom domains. The second base argument is only used if the first argument doesn't contain a host, e.g.

> (new URL('https://mysite.com/remote.html', 'https://playground.wordpress.net')).toString()
https://mysite.com/remote.html

The opfs integration is brilliant by the way. It is so much better than shared array buffer to integrate with.

❤️ YAY, thank you!

I would love a hint from the code on how you got that to work with the emscriptem fs by the way?????

It's a trick! :-) OPFS is asynchronous and I couldn't get it to work with Emscripten without shared array buffers, so I did this:

https://github.com/WordPress/wordpress-playground/blob/a30405b8324155ebc424eb608b4e83bb0441596b/packages/playground/sync/src/setup-playground-sync.ts

tl;dr – every MEMFS operation (create, delete, move etc) is tracked and repeated in OPFS. The "Sync changes from OPFS" button iterates through the entire OPFS handle and overwrites everything in MEMFS with OPFS data.

const qs = new URLSearchParams(urlObject.search);
for (const [key, value] of Object.entries(params)) {
if (value !== undefined && value !== null && value !== false) {
Expand Down
16 changes: 16 additions & 0 deletions packages/playground/remote/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="utf-8" />
<meta name="viewport" content="width=device-width, initial-scale=1" />
<title>Wordpress for teaching</title>
<link href="./src/style.css" rel="stylesheet" />
</head>

<body>
<div><x-wordpress /></div>
<p style="float: right">Version: <x-version /></p>
<script src="https://wp-mfe.pages.dev/bundle.js"></script>
<script src="./src/version.ts" type="module"></script>
</body>
</html>
Comment on lines +1 to +16
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems specific to the Wordpress for teaching project and shouldn't be part of core Playground.

2 changes: 1 addition & 1 deletion packages/playground/remote/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@wp-playground/remote",
"version": "0.0.1",
"version": "0.0.5",
"description": "WordPress Playground remote host",
"repository": {
"type": "git",
Expand Down
21 changes: 20 additions & 1 deletion packages/playground/remote/service-worker.ts
View file
Open in desktop
Copy link
Author

Choose a reason for hiding this comment

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

@adamziel , @bgrgicak do you have an opinion on this. I had broken image and anchor links due to the old scopes sticking around. I made the links relative and put in the new scope and it seemed to fix the problem.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What part of the code are you referring to?

Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,32 @@
import { applyRewriteRules } from '@php-wasm/universal';
import {
awaitReply,
convertFetchEventToPHPRequest,
convertFetchEventToPHPRequest as convertFetchEvent,
initializeServiceWorker,
cloneRequest,
broadcastMessageExpectReply,
} from '@php-wasm/web-service-worker';
import { wordPressRewriteRules } from '@wp-playground/wordpress';

async function convertFetchEventToPHPRequest(event: any) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this needed? Why not make the changes in @php-wasm/web-service-worker?

const fullUrl = new URL(event.request.url);
const scope = getURLScope(fullUrl);
const res: any = await convertFetchEvent(event);
const contentType: string = res.headers.get('content-type');
if (contentType?.match(/text\/html/)) {
const canonical: string = await res.text();
let relative = canonical.replace(
/(http|https):[\/\\]+(localhost|127.0.0.1|playground\.wordpress\.net|diy-pwa\.com)[:0-9]*\/scope:\d.\d*/g,

Check failure on line 24 in packages/playground/remote/service-worker.ts

View workflow job for this annotation

GitHub Actions / Lint and typecheck 

Unnecessary escape character: \/
Copy link
Collaborator

Choose a reason for hiding this comment

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

We shouldn't have anything domain-specific in the converter. Why do these domains need to be threaded differently from other domains?

`/scope:${scope}`
);
relative = relative.replace(/http:/g, 'https:');
const resNew = new Response(relative, res);
return resNew;
} else {
return res;
}
}

if (!(self as any).document) {
// Workaround: vite translates import.meta.url
// to document.currentScript which fails inside of
Expand Down
14 changes: 14 additions & 0 deletions packages/playground/remote/src/style.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
body {
margin: 0;
}

iframe {
display: block;
/* iframes are inline by default */
background: #000;
border: none;
/* Reset default border */
height: 100vh;
/* Viewport-relative units */
width: 100vw;
}
9 changes: 9 additions & 0 deletions packages/playground/remote/src/version.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import info from '../package.json' assert { type: 'json' };

class VersionNumber extends HTMLElement {
connectedCallback() {
this.innerHTML = info.version;
}
}

customElements.define('x-version', VersionNumber);
1 change: 1 addition & 0 deletions packages/playground/remote/vite.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ export default defineConfig({
assetsInlineLimit: 0,
rollupOptions: {
input: {
index: path('/index.html'),
wordpress: path('/remote.html'),
},
},
Expand Down