-
Notifications
You must be signed in to change notification settings - Fork 249
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
base: trunk
Are you sure you want to change the base?
Changes from all commits
847fcc3
b3d0e61
67bcca1
7e84060
2d076ef
05fe606
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -79,7 +79,6 @@ export async function startPlaygroundWeb({ | |
onClientConnected = () => {}, | ||
sapiName, | ||
}: StartPlaygroundOptions): Promise<PlaygroundClient> { | ||
assertValidRemote(remoteUrl); | ||
allowStorageAccessByUserActivation(iframe); | ||
|
||
remoteUrl = setQueryParams(remoteUrl, { | ||
|
@@ -173,22 +172,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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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????? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, that check should only kick in for > (new URL('https://mysite.com/remote.html', 'https://playground.wordpress.net')).toString()
https://mysite.com/remote.html
❤️ YAY, thank you!
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: 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) { | ||
|
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
---|---|---|
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this needed? Why not make the changes in |
||
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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
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; | ||
} |
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Potentially, yes! I'm just not sure what do you mean by a custom element – would you elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please send me a message on the Making WordPress Slack (@bero) I'm happy to schedule some time to chat.
There was a problem hiding this comment.
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.