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

Run examples with php wasm #1097

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

Conversation

soyuka
Copy link

@soyuka soyuka commented Oct 16, 2024

Adds a PHP Wasm binary to run documentation examples in the browser. We discussed this with @pronskiy (
fixes php/doc-en#3259).

php-doc-2.mp4

I've builtin PHP wasm 8.4.0-dev and I'm not sure what would be preferred to keep this up to date. If you want to reproduce the build see:

soyuka/php-wasm#7

Clone that branch and execute:

docker buildx bake
cp build/* ../phpdoc/web-php/js

Work sponsored by Les-Tilleuls.coop inline with what we built at the API Platform Playground more informations on a blog post

Copy link

github-actions bot commented Oct 16, 2024

🚀 Regression report for commit 9fc3eff is at https://web-php-regression-report-pr-1097.preview.thephp.foundation

Copy link

github-actions bot commented Oct 16, 2024

🚀 Preview for commit 9fc3eff can be found at https://web-php-pr-1097.preview.thephp.foundation

@soyuka

This comment was marked as duplicate.


async function main() {
const buffer = [];
const {ccall} = await phpBinary({
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const {ccall} = await phpBinary({
const {ccall} = await phpBinary({

(mixed tabs and spaces)

Comment on lines 37 to 39
document.querySelectorAll('.example').forEach((example) => {
const button = document.createElement('button');
const phpcode = example.querySelector('.phpcode');
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
document.querySelectorAll('.example').forEach((example) => {
const button = document.createElement('button');
const phpcode = example.querySelector('.phpcode');
document.querySelectorAll('.example').forEach((example) => {
const button = document.createElement('button');
const phpcode = example.querySelector('.phpcode');

Comment on lines 44 to 45
button.innerText = 'Run code';
button.onclick = function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
button.innerText = 'Run code';
button.onclick = function() {
button.innerText = 'Run code';
button.onclick = function() {

Comment on lines 54 to 57
};

phpcode.parentNode.insertBefore(button, phpcode);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
};
phpcode.parentNode.insertBefore(button, phpcode);
});
};
phpcode.parentNode.insertBefore(button, phpcode);
});

Comment on lines 4 to 16
const container = document.createElement('div');
container.classList.add('screen', 'example-contents');
const title = document.createElement('p');
title.innerText = 'Output: ';
container.appendChild(title)
const div = document.createElement('div');
div.classList.add('examplescode');
container.appendChild(div);
const pre = document.createElement('pre');
pre.classList.add('examplescode');
pre.innerText = output;
div.appendChild(pre)
return container;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const container = document.createElement('div');
container.classList.add('screen', 'example-contents');
const title = document.createElement('p');
title.innerText = 'Output: ';
container.appendChild(title)
const div = document.createElement('div');
div.classList.add('examplescode');
container.appendChild(div);
const pre = document.createElement('pre');
pre.classList.add('examplescode');
pre.innerText = output;
div.appendChild(pre)
return container;
const container = document.createElement('div');
container.classList.add('screen', 'example-contents');
const title = document.createElement('p');
title.innerText = 'Output: ';
container.appendChild(title);
const div = document.createElement('div');
div.classList.add('examplescode');
container.appendChild(div);
const pre = document.createElement('pre');
pre.classList.add('examplescode');
pre.innerText = output;
div.appendChild(pre);
return container;

CS

button.innerText = 'Run code';
button.onclick = function() {
if (lastOutput && lastOutput.parentNode) {
lastOutput.parentNode.removeChild(lastOutput)
Copy link

@stof stof Oct 16, 2024

Choose a reason for hiding this comment

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

much simpler: lastOutput.remove() (there is no reason to care about unmaintained browsers not supporting the remove method when you are adding a feature requiring WebAssembly to work)

let lastOutput = null

document.querySelectorAll('.example').forEach((example) => {
const button = document.createElement('button');
Copy link

Choose a reason for hiding this comment

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

the created button should use the button type instead of the default submit type, to avoid issues in case this ends up being used in a place being inside a form.

lastOutput.parentNode.removeChild(lastOutput)
}

ccall("phpw_run", null, ["string"], ['?>' + phpcode.innerText]);
Copy link

Choose a reason for hiding this comment

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

should this use .innerText or .textContent here ? innerText is impacted by the styles of the page

Copy link
Author

Choose a reason for hiding this comment

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

no idea 🤷

buffer.length = 0;
};

phpcode.parentNode.insertBefore(button, phpcode);
Copy link

Choose a reason for hiding this comment

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

use phpcode.before() instead

@stof
Copy link

stof commented Oct 16, 2024

Disabling many extensions as you did in soyuka/php-wasm#7 is likely to make many examples non working though (in your build, even JSON functions are not available)

@soyuka
Copy link
Author

soyuka commented Oct 16, 2024

Disabling many extensions as you did in soyuka/php-wasm#7 is likely to make many examples non working though (in your build, even JSON functions are not available)

Definitely, there are a few things to discuss, I can enable most extensions, some will require additional libraries (for example dom or xml require libxml) and it'll add weight to the build (now it's "only" 3.6 MB).

$path = dirname(__DIR__) . '/js/' . $filename;
echo '<script type="module" src="/cached.php?t=' . @filemtime($path) . '&amp;f=/js/' . $filename . '"></script>' . "\n";
}
?>
Copy link
Author

Choose a reason for hiding this comment

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

maybe that this deserves only one line?

@pronskiy
Copy link
Contributor

As I understand, php binary is always loaded now, right? If yes, does it make sense to load it only when the user clicks "Run" for the first time?

@soyuka
Copy link
Author

soyuka commented Oct 17, 2024

done

@soyuka
Copy link
Author

soyuka commented Oct 17, 2024

Note that we should use brotli or gzip though, I'm unfamiliar with the server architecture behind the php docs website, should I add this directly to the cached.php file?

@Girgias
Copy link
Member

Girgias commented Oct 17, 2024

@derickr is likely the person that would know this.

@jimwins
Copy link
Member

jimwins commented Oct 17, 2024

It looks like php.net isn't particularly well set-up for encoding content. You can see how CSS/JS/etc is served up by the cached.php file, and testing with curl --compressed -I shows that it doesn't get encoded. HTML content does get gzip-encoded, at least.

I'm not sure how much of the encoding happens on our server vs. what Myra does on the edge, but there's probably room for improvement there even outside of encoding the WASM bundle appropriately. The current web configuration isn't in the systems repo yet, though.

@lwlwilliam
Copy link

lwlwilliam commented Oct 18, 2024

If the modified code can still be highlighted, that would be even better.

@stof
Copy link

stof commented Oct 18, 2024

@lwlwilliam this would require bringing an actual code editor (Codemirror or Monaco) instead of just using contenteditable

@pronskiy
Copy link
Contributor

@lwlwilliam, @stof, it makes sense for sure, but perhaps as step two in a separate PR? Just to keep this one smaller and delivered sooner. I'm pretty sure some unforeseen bugs will be discovered when this is deployed, and it'll be easier to fix them with smaller steps.

@pronskiy
Copy link
Contributor

@soyuka, what do you think about adding the PHP version in the output label?
Screenshot 2024-10-21 at 11 42 10 AM

@derickr
Copy link
Member

derickr commented Oct 21, 2024

Myra already caches cached.php with a 2 minute TTL (and 1 minute in case of a 404), but there are no settings for specific compressions:

prefix	/cached.php	2 minutes	1 minute

I can ask whether they can do on the fly-compression, but I am not sure whether they do.

To answer @jimwins, the config isn't in the systems repo, as nothing on php-web4 has been "php-ified" yet. I think Sascha set it up with nginx in his own way. There is no /local/systems either. The nginx configuration does not have any compression options set either.

I believe adding it to cached.php with the right headers (content-type and compression-type), with storing the compressed file should work. You should be able to test that yourself on your local development environment too.

@soyuka
Copy link
Author

soyuka commented Oct 21, 2024

I believe adding it to cached.php with the right headers (content-type and compression-type), with storing the compressed file should work. You should be able to test that yourself on your local development environment too.

I gave a try
diff --git a/cached.php b/cached.php
index 766afb2e..c8055ef7 100644
--- a/cached.php
+++ b/cached.php
@@ -46,6 +46,14 @@
     header("Content-Type: application/javascript");
 } elseif (substr($abs, -4) == ".css") {
     header("Content-Type: text/css");
+} elseif (substr($abs, -4) == ".wasm") {
+	header("Content-Type: application/wasm");
+}
+
+$gzipped = $abs . '.gz';
+if (file_exists($gzipped) && isset($_SERVER['HTTP_ACCEPT_ENCODING']) && false !== strpos($_SERVER['HTTP_ACCEPT_ENCODING'], 'gzip')) {
+    $abs = $gzipped;
+	header("Content-Encoding: gzip");
 }
 
 readfile($abs);

But as the Javascript is requiring the php-web.wasm file it's not ideal and should be left to the HTTP server in my opinion. For example, I'm running the docs using FrankenPHP:

20241021_21h20m16s_grim

@pronskiy added version:

20241021_21h22m16s_grim

Co-authored-by: lwlinux <[email protected]>
@derickr
Copy link
Member

derickr commented Oct 28, 2024

I don't quite understand whether your "try" was succesful or not. I can only see an image.

The javascript should surely be calling /cached.php?.... and not the .wasm file directly...?

And we deploy on nginx, so you should test with that, and not with something else. I have just added the nginx configuration to the systems repository: https://github.com/php/systems/tree/master/php-web4/nginx

@shaedrich
Copy link

@pronskiy added version:

Should there be only one version or all of the supported in a dropdown/select?

@pronskiy
Copy link
Contributor

@shaedrich, for the sake of simplicity of this first step, it's better to stick to just one version. The dropdown sounds like a good idea for the next step.

@cookieguru
Copy link
Contributor

Probably want to add

js/php-web.wasm filter=lfs diff=lfs merge=lfs binary

to .gitattributes

@pronskiy
Copy link
Contributor

@derickr, @saundefined, @sy-records, folks, do you think this is ready to merge?

Copy link
Member

@sy-records sy-records left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Member

@derickr derickr left a comment

Choose a reason for hiding this comment

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

I am getting failures when I try to check out this branch:

remote: Enumerating objects: 31, done.
remote: Counting objects: 100% (31/31), done.
remote: Compressing objects: 100% (15/15), done.
remote: Total 31 (delta 17), reused 27 (delta 16), pack-reused 0 (from 0)
Unpacking objects: 100% (31/31), 1.20 MiB | 1.79 MiB/s, done.
From https://github.com/php/web-php
 * [new ref]           refs/pull/1097/head -> pr/1097
M	distributions
Switched to branch 'pr/1097'
Encountered 1 file that should have been a pointer, but wasn't:
	js/php-web.wasm
error: cannot rebase: You have unstaged changes.
error: Please commit or stash them.

It is perhaps because something is on LFS, or not? The file just looked checked in here, so what is the deal with adding lfs to .gitattributes?

If it is LFS, then you need to provide instructions as to how to check it out properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Interactive code examples on php.net