-
Notifications
You must be signed in to change notification settings - Fork 269
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
Add terminal to playground site #161
Conversation
swissspidy
commented
Mar 18, 2023
•
edited
Loading
edited
Thank you @swissspidy ! GitHub is going through some outage and mostly not accepting my comments so I'll review it tomorrow |
@@ -200,7 +200,7 @@ const char WASM_HARDCODED_INI[] = | |||
"always_populate_raw_post_data = -1\n" | |||
"upload_max_filesize = 2000M\n" | |||
"post_max_size = 2000M\n" | |||
"disable_functions = proc_open,popen,curl_exec,curl_multi_exec\n" | |||
"disable_functions = proc_open,popen,curl_exec,curl_multi_exec,shell_exec,proc_close\n" |
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.
"disable_functions = proc_open,popen,curl_exec,curl_multi_exec,shell_exec,proc_close\n" | |
"disable_functions = proc_open,popen,curl_exec,curl_multi_exec,proc_close\n" |
It's possible to provide a custom JavaScript handler for shell_exec
so let's only soft-disable it in php.ini. This way it's possible to re-enable it in runtime.
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.
Is the same possible also for proc_open
? Overriding that one would be needed for making Behat tests work.
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.
Not at the moment. shell_exec
was easier to handle because it returns the output as a string. proc_open
returns a resource and exposes low-level process calls and will require more effort. Definitely possible to support, though.
WITH_LIBXML: 'yes', | ||
WITH_MBSTRING: 'yes', |
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.
Building with libxml adds 1MB to the bundle size, but I realize it's critical for this PR to be useful 🤔
I need to think a bit more about this. Emscripten seems to support runtime dynamic linking, but it's a small project in itself I don't want to block this PR on it. Maybe an additional MB wouldn't be that bad for now.
Only got so far today. I'll continue reviewing tomorrow. |
@@ -22,6 +22,32 @@ RUN wget -O wp.zip $WP_ZIP_URL && \ | |||
unzip -q wp.zip && \ | |||
rm wp.zip | |||
|
|||
# Run extra steps for the develop branch |
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.
The more these Dockerfiles grow the more I want to migrate to a Makefile down the road. Nothing blocking here, just sharing.
@@ -37,40 +63,57 @@ RUN git clone https://github.com/WordPress/sqlite-database-integration.git \ | |||
# Required by the SQLite integration plugin: | |||
cp wordpress/wp-config-sample.php wordpress/wp-config.php | |||
|
|||
# Enable the SQLite support for the src folder in wordpress-develop: | |||
|
|||
RUN if [[ -d wordpress-develop-trunk ]] ; then \ |
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.
This RUN is mostly the same as the one above, perhaps merging them and exporting a PATH_PREFIX inside of an if/else
would make it shorter? I'm happy to fiddle with that if you invite me to your fork.
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.
You should already have access according to the "Allow edits by maintainers" checkbox, so feel free to make any edits
@@ -129,15 +174,44 @@ RUN ls -la && \ | |||
exit 'WordPress installation failed'; \ | |||
fi | |||
|
|||
# === Run WordPress Installer for the src install === | |||
#RUN if [[ -d wordpress-develop-trunk ]] ; then \ |
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.
Is this code path used in wordpress-develop
build? If so, https://github.com/WordPress/wordpress-playground/pull/161/files#r1152126565 it could likely be merged with the RUN above into a single RUN
. Again, I'm happy to play with it.
@@ -3,6 +3,11 @@ import { spawn } from 'child_process'; | |||
import yargs from 'yargs'; | |||
|
|||
const presets = { | |||
develop: { |
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.
Nit: formatting. Running nx format
and npm run lint
should fix it automatically.
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.
This will keep coming up in other PRs so I should just add a pre-commit hook. I really dislike lint-staged because of how many times it put me in a detached HEAD state, but Husky alone should suffice since nx is smart enough to ignore unchanged files.
@@ -3,6 +3,11 @@ import { spawn } from 'child_process'; | |||
import yargs from 'yargs'; | |||
|
|||
const presets = { | |||
develop: { | |||
KEEP_THEME: 'twentytwentythree', | |||
WP_ZIP_URL: 'https://github.com/WordPress/wordpress-develop/archive/refs/heads/trunk.zip', |
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.
Smart!
return null; | ||
} | ||
|
||
const extensions = { |
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.
For posterity: Later on this and #168 may converge into a single CodeMirror
. For now, let's ship both.
|
||
function proc_open( $command, $descriptor_spec, $pipes ) { return false; } | ||
|
||
define('STDIN', fopen('php://stdin', 'rb')); |
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.
Future idea: provide php.define( name, value )
and php.defineStdio()
scriptPath: '/wordpress/run-tests.php' | ||
}); | ||
|
||
const decodedOutput = new TextDecoder().decode(output.body); |
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.
Next iteration idea: Stream the output live. This will require a function like php.outputMode( 'file' | 'callback' )
import React, {useCallback, useEffect, useRef, useState} from 'react'; | ||
import css from './style.module.css'; | ||
import {PlaygroundClient} from '@wp-playground/playground-remote'; | ||
import {Terminal} from 'xterm'; |
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.
64kb uncompressed, maybe import asynchronously? Vite will handle code splitting when it finds import('xterm')
contentLabel='This is a dialog window which overlays the main content of the page.' | ||
onRequestClose={closeModal} | ||
style={{ | ||
content: { |
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.
Note to self: Extract a generic PlaygroundModal component to avoid duplicating these styles.
@@ -0,0 +1,22 @@ | |||
.btn { |
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.
Note to self: Extract a generic Button component to avoid copying and pasting the same CSS
@@ -37,15 +37,11 @@ interface UsePlaygroundOptions { | |||
onConnected?: (playground: PlaygroundClient) => Promise<void>; | |||
onConnectionTimeout?: () => void; | |||
timeout?: number; | |||
php?: string; |
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.
good catch
It reads well, thank you @swissspidy! I left some notes for now and some future ideas. I'm happy to tackle any of these, too. I tried running the PR locally but Vite complained about the missing
From reading the code, that's exactly what I had in mind. Thank you! Having a separate entrypoint keeps the code editor in the repo, enables using it on demand, and helps incubate it over time. |
Thanks for your thorough review!
I'll be on vacation soon, so I'll take any help I can get with this :)
Docroot is right now hardcoded to |
Oh yay, have fun! Would you please invite me to your fork? Otherwise I'll have to start a new PR to be able to make changes.
Oh that's right, thank you |
Done! |
Adds `wp-cli` support in the browser! ![CleanShot 2024-01-22 at 15 13 38@2x](https://github.com/WordPress/wordpress-playground/assets/205419/4f0a352f-8e2f-4961-b0c4-a9e88804931e) This PR brings parts of #161 by @swissspidy into `trunk` to enable using `wp-cli` in the browser. ## Codebase changes * `setSapiName` method for PHP. `wp-cli` expects to see the exact string `"cli"` as the SAPI name. * All PHP versions are now built with phar support. * `js_open_process` is asynchronous via Asyncify. This is to enable calling `setSpawnHandler(handler)` from the parent site to set a handler in an asynchronous web worker. Unfortunately, using the Comlink messaging library is challenging with complex objects like spawn handlers so this PR ships a workaround where spawn handlers are passed as strings and `eval()`-ed in the worker. This shouldn't have any security implications as Playground enables evaluating arbitrary code by definition. * `proc_open` waits for the process to spawn, and `proc_close` waits for the process to die. This is necessary to support the `wp-cli` behavior of piping its output through a pager program (like `cat` or `less -S`). ## Follow-up work * Remove `eval()` from `setSpawnHandler(handler)`. Setting spawn handler works in Node.js where PHP runs synchronously in the same process, but these spawn handlers are challenging to transfer over `postMessage` as they're essentially composite EventEmitters that need to accept input inside the Worker process, handle it in the client process, and transmit it back to the worker process. Intuitively, it would be pretty easy if we used `MessagePort` directly. It seems like `Comlink` as an abstraction layer makes things much easier to a point, since we can magically call functions on objects living in another thread, but there's a boundary beyond which Comlink makes these API interactions more difficult. cc @danielbachhuber @schlessera
Superseded by ae8ec6a |