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.

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

changes to use wordpress-playground for teaching #1145

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

Conversation

rhildred
Copy link

What is this PR doing?

This pull request is composed of 3 main changes that I needed to make to host my own version at https://diy-pwa.com and consume @php-wasm/node in a github action.

What problem is it solving?

Make remote.html have the same origin as my microfrontend so that I can have a microfrontend that does github integration using opfs.

How is the problem addressed?

Remove the official playground.wordpress.net url from the client and use window.location.href as the base for new URL. There is also service worker code to make urls be relative rather than canonical so that .zip files from various sources can be restored.

Testing Instructions

To experience the github integration, create a repository with a README.md. In the README.md have an edit link:

[Edit here](https://diy-pwa.com/~/rhildred/March25)

for instance. You will need a classic developer token to be able to push to the remote repository.

Thanks so much for wordpress-playground. I used it with my students all term. Students were able to add a github action to publish their wordpress sites on github pages. That way they could showcase multiple works in their portfolios.

@rhildred rhildred requested a review from a team as a code owner March 26, 2024 21:12
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.

"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

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

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

Comment on lines +1 to +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>
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.

@bgrgicak
Copy link
Collaborator

bgrgicak commented Apr 3, 2024

@rhildred some of the changes in this PR seem specific to your project. We would love for Playground to support your work, but we can't add code that is specific to a single project to the core Playground repository.

In case you need to override some core behavior, we should ensure Playground is flexible enough to support it.
What flexibility are you missing in Playground today to achieve your goals?

I would love to understand more about your project to see how we can help you.

If you ever want to chat, feel free to reach out to us on the Making WordPress Slack. I'm happy to schedule some time to talk.

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

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.

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?

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?

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

@adamziel adamziel force-pushed the trunk branch 2 times, most recently from 680cd19 to 2e376d2 Compare October 4, 2024 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants