Remove DOMParser from Blueprints: installPlugin and installTheme #427

eliot-akira · 2023-05-24T01:24:29Z

What?

The goal of this pull request is to remove the use of any browser-specific API such as DOMParser from the Blueprints package. This allows all Blueprint steps to run on the browser and server side.

Why?

Currently, the steps installPlugin and installTheme can only be run with the Playground in the browser, not on server side with wp-now and Node.js, because they use the asDOM helper function which depends on the DOMParser class only available in the browser.

Related discussion:

Blueprints: Isomorphic DOM - DOMParser, FormData, fetch #379

The initial idea was to introduce an "isomorphic DOM" library that exports native DOM API for the browser and jsdom for Node.js. That would have allowed the existing implementation of installPlugin and installTheme to work as is on server side.

However, after weighing the pros and cons, it was decided that it's simpler to maintain if we rewrite these steps to perform their actions without using any DOM operations.

How?

Rewrite the Blueprint steps installPlugin and installTheme to use Playground and PHP WASM API.
Remove the asDOM helper function.

Testing Instructions

Check out the branch.
Run npx nx test playground-blueprints

packages/playground/blueprints/src/lib/steps/install-plugin.ts

eliot-akira · 2023-05-24T12:17:21Z

@adamziel Thank you for reviewing this rough draft.

I didn't know about the unzip step, that will save me the effort of manually extracting the zip file. Yes, I'll make sure to get the correct plugin folder name after extraction, since it's often different from the zip file name. And will delete the zip file afterwards.

I started the PR by writing an automated test, but realized that the Playground on Node.js side, @wp-playground/node, is not quite ready to be used. (I'm guessing the work being done in wp-now could be useful eventually, for server-side tests to make use of a full WordPress environment.) Also, when I imported the package from the test, I got an error from nx or vitest, "Could not execute command because the task graph has a circular dependency" - because that package imports blueprints. There's probably a way to solve it, but I decided to leave that for later.

So, for now, the test will be bare bones with PHP WASM, only to confirm that the plugin was extracted to the expected location. I'll do some manual tests on the browser side for plugin activation, which requires WordPress.

In the CI logs, you can see that the call to writeFile is failing. https://github.com/WordPress/wordpress-playground/actions/runs/5063668857/jobs/9090521404?pr=427#step:9:86

TypeError: file.arrayBuffer is not a function
    at Module.fileToUint8Array (/home/runner/work/wordpress-playground/wordpress-playground/packages/playground/blueprints/src/lib/steps/common.ts:29:35)
    at Module.writeFile (/home/runner/work/wordpress-playground/wordpress-playground/packages/playground/blueprints/src/lib/steps/client-methods.ts:292:16)
    at Module.installPlugin (/home/runner/work/wordpress-playground/wordpress-playground/packages/playground/blueprints/src/lib/steps/install-plugin.ts:72:9)

Apparently it's because the variable pluginZipFile is supposed to be an instance of File, but it's lacking the arrayBuffer method. That variable came from resolving an instance of VFSResource, I believe somewhere in runBlueprintSteps, and when I dump it to console it does seem to be an actual File instance. So I'm trying to figure out how to convert or extract from pluginZipFile to pass it in a form that writeFile expects.

Working on this PR is turning out to be a good excursion through the codebase, learning about various new aspects of the project.

eliot-akira · 2023-05-24T20:57:35Z

the call to writeFile is failing .. because pluginZipFile is supposed to be an instance of File, but it's lacking the arrayBuffer method. That variable came from resolving an instance of VFSResource

Still trying to figure this out. It's definitely an instance of File, coming from VFSResource.resolve.

return new File([buffer], this.name);

The buffer is Uint8Array with data in it. I changed the line to:

const file = new File([buffer], this.name);
console.log(await file.arrayBuffer());
return file;

And get the same error:

TypeError: file.arrayBuffer is not a function

Weird. I'm going to try using LiteralResource in the test instead.

EDIT: Same error with literal resource passing raw buffer as contents.

Looks like the File class in Node.js was in experimental status until a couple months ago (March 2023).

buffer: graduate File from experimental and expose as global nodejs/node#47153

It extends the Blob class, which is supposed to have an arrayBuffer method. Possibly related issues:

eliot-akira · 2023-05-24T21:50:17Z

On the browser side, it's getting close. File.arrayBuffer() is working as expected.

At the last step of the install process, this line:

await playground.mv(tmpPluginPath, pluginPath);

Throws:

TypeError: this[__private__dont__use].FS.mv is not a function
    at WebPHP.mv (VM762 base-php.ts:398:35)
    at descriptor.value (VM764 rethrow-file-system-error.ts:85:23)
    at PlaygroundWorkerEndpoint.mv (VM773 web-php-endpoint.ts:28:35)
    at Proxy.<anonymous> (VM770 api.ts:89:43)
    at callback (VM772 comlink.js:89:36)

EDIT: The FS method is called rename, not mv, in both Emscripten and Node.js.

…urn promise

packages/php-wasm/universal/src/lib/base-php.ts

README.md

packages/playground/blueprints/src/lib/steps/install-plugin.ts

adamziel · 2023-05-24T23:05:08Z

I started the PR by writing an automated test, but realized that the Playground on Node.js side, @wp-playground/node, is not quite ready to be used.

Yes, that’s definitely something to improve. Perhaps it will make sense to put parts of wp-now there.

(I'm guessing the work being done in wp-now could be useful eventually, for server-side tests to make use of a full WordPress environment.)

A-ha! That’s an interesting idea!

When I imported the package from the test, I got an error from nx or vitest, "Could not execute command because the task graph has a circular dependency" - because that package imports blueprints.

That’s a challenging one. Perhaps building wp-now to a single bundle and then using that to run Blueprints tests would solve it here. This is the second time this comes up today- the first time was in #434

Co-authored-by: Adam Zielinski <adam@adamziel.com>

…to provide missing method File.arrayBuffer()

…as been fixed upstream

eliot-akira · 2023-05-24T23:46:04Z

OK, the server-side test is now passing. I had to patch VFSResource to provide File.arrayBuffer() if it's missing - kinda ugly but I think this is an issue in Node.js itself.

On the browser side, it looks like activatePlugin step is not working, even though the extracted plugin folder exists. Will look into it.

I've prepared a separate little PR for FS.rename.

BasePHP: Use FS.rename for mv method #445

packages/playground/blueprints/src/lib/steps/activate-plugin.ts

…e or relative path)

adamziel · 2023-05-31T20:45:50Z

packages/playground/blueprints/src/lib/steps/common.ts

+ * Import the polyfilled File class below to ensure its buffer is available to
+ * functions like writeFile (./client-methods.ts) and fileToUint8Array (above).
+ */
+class FilePolyfill extends File {


Ideally this would live in tests, but that makes importing Blueprints in other packages tricky as every package would have to polyfill it. I think this is a neat solution, thank you for taking the time to explore this 👍

adamziel · 2023-05-31T20:49:21Z

packages/playground/blueprints/src/lib/steps/install-asset.ts

+/**
+ * Install asset: Extract folder from zip file and move it to target
+ */
+export async function installAsset(


I initially had this in mind as an internal utility to aid other steps, I wonder whether this makes sense as its own step 🤔

The term "asset" seems ambiguous as part of a public API. Internally it means:

a zip package

with a single folder of files in it

to be extracted to a target path

Technically it's generic, not limited to plugins and themes - but I can't imagine what else would be considered "assets" that has the above characteristics.

There could be an installMuPlugin blueprint step, same as for plugins but with target mu-plugins.

Media files could be another kind of asset. Maybe an installMedia step that uses installAssets. Unlike plugins/themes, it would not require a single container folder. And the step will need to run additional PHP to upload the files into the media library.

So I think installAsset by itself may not be useful to expose as a blueprint step, and can probably stay an internal helper function.

Oh sorry I got confused. It is in the steps directory and kind of looks like a step declaration, but it actually is an internal helper. My bad!

It would be nice to put each step in a separate file and move helpers somewhere distinct, maybe a subdirectory? Let's figure this out separately

packages/playground/blueprints/src/lib/steps/activate-plugin.ts

adamziel · 2023-05-31T20:52:04Z

This is looking great @eliot-akira! Two more comments from me, otherwise it looks ready.

…PHP error

adamziel

Really great work here @eliot-akira ! Now these can be used outside of the browser and power the CLI tools!

eliot-akira · 2023-05-31T23:14:39Z

Thank you for a thorough review of the PR. That was good exercise and satisfying to see the code get refined through discussion.

I discovered there's another significant use of DOMParser in the importFile step (steps/import-export.ts). It's used for interacting with admin screens of the Importer to upload form data. Maybe difficult but surely it's possible to rewrite it to be universal for browser and server. Will leave that for another occasion.

## What? Add `prependPath` option to `PHP.listFiles` method, which will prepend given folder path to every file found in it. ## Why? It's a common need to iterate over a list of files with each file path being an accessible path instead of only the file name. Idea mentioned in: - #427 (comment) For example, this is a common pattern: ```ts const files = await playground.listFiles( folderPath ) for (const file of files) { const filePath = `${folderPath}/${file}`; ... } ``` Also expressed as: ```ts const filePaths = (await playground.listFiles( folderPath )).map( (name: string) => `${folderPath}/${name}`) ) ``` With the new option, the above can be simplified as: ```ts const filePaths = await playground.listFiles(folderPath, { prependPath: true }) ``` ## How? - [x] Add `prependPath` option to `BasePHP.listFiles` method - [x] Document the option and what it does - [x] Add test ## Testing Instructions  1. Check out the branch. 2. Run `nx test playground-blueprints`

Installing a plugin where the plugin files are directly at the top level does not work. This PR assumes that, unless the zip file contains only a single directory, the plugin files start at the top level. Related: #427 Unit tests included. Run `npm run dev` and confirm that adding ?gutenberg-pr=47739 to the URL installs the PR.

## What? Add `prependPath` option to `PHP.listFiles` method, which will prepend given folder path to every file found in it. ## Why? It's a common need to iterate over a list of files with each file path being an accessible path instead of only the file name. Idea mentioned in: - WordPress/wordpress-playground#427 (comment) For example, this is a common pattern: ```ts const files = await playground.listFiles( folderPath ) for (const file of files) { const filePath = `${folderPath}/${file}`; ... } ``` Also expressed as: ```ts const filePaths = (await playground.listFiles( folderPath )).map( (name: string) => `${folderPath}/${name}`) ) ``` With the new option, the above can be simplified as: ```ts const filePaths = await playground.listFiles(folderPath, { prependPath: true }) ``` ## How? - [x] Add `prependPath` option to `BasePHP.listFiles` method - [x] Document the option and what it does - [x] Add test ## Testing Instructions  1. Check out the branch. 2. Run `nx test playground-blueprints`

eliot-akira added 2 commits May 24, 2023 02:35

Rough draft for installPlugin

815acec

Format

61e33f3

eliot-akira changed the title ~~Remove DOMParser from Blueprints~~ Draft: Remove DOMParser from Blueprints May 24, 2023

adamziel reviewed May 24, 2023

View reviewed changes

packages/playground/blueprints/src/lib/steps/install-plugin.ts Outdated Show resolved Hide resolved

adamziel reviewed May 24, 2023

View reviewed changes

packages/playground/blueprints/src/lib/steps/install-plugin.ts Show resolved Hide resolved

eliot-akira added 3 commits May 24, 2023 21:40

Merge remote-tracking branch 'upstream/trunk'

7eb0e26

Test zip package name different from extracted plugin folder name

304e256

Remove test plugin after install

9c34cf4

eliot-akira added 3 commits May 24, 2023 23:22

Use unzip step and find extracted plugin folder name

89a627d

Format

d53a55a

playground.documentRoot getter returns a promise

98cde65

eliot-akira added 4 commits May 24, 2023 23:53

Remove temporary zip file after extraction

bfaef98

Format

3f8eb82

activatePlugin step: await playground.documentRoot because it can ret…

f2ed29b

…urn promise

BasePHP: Use FS.rename for method mv

7840907

eliot-akira commented May 24, 2023

View reviewed changes

packages/php-wasm/universal/src/lib/base-php.ts Outdated Show resolved Hide resolved

adamziel reviewed May 24, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

adamziel reviewed May 24, 2023

View reviewed changes

packages/playground/blueprints/src/lib/steps/install-plugin.ts Outdated Show resolved Hide resolved

adamziel reviewed May 24, 2023

View reviewed changes

packages/playground/blueprints/src/lib/steps/install-plugin.ts Show resolved Hide resolved

eliot-akira and others added 4 commits May 25, 2023 01:14

Improve error message when extracted plugin folder not found

daf74b3

Co-authored-by: Adam Zielinski <adam@adamziel.com>

Make server-side test pass: Create plugins folder; Patch VFSResource …

82ad36e

…to provide missing method File.arrayBuffer()

Format

3520de3

Add comment about Gutenberg patch becoming unnecessary as the issue h…

579c568

…as been fixed upstream

eliot-akira added 2 commits May 25, 2023 02:19

Rewrite installTheme step

534d10f

Format

0615596

eliot-akira changed the title ~~Remove DOMParser from Blueprints~~ Remove DOMParser from Blueprints: installPlugin and installTheme May 31, 2023

dmsnell reviewed May 31, 2023

View reviewed changes

packages/playground/blueprints/src/lib/steps/activate-plugin.ts Outdated Show resolved Hide resolved

eliot-akira added 8 commits May 31, 2023 16:12

Polyfill the File class in JSDOM which lacks arrayBuffer() method

3b67796

Format

c121866

activatePlugin: Simplify logic to find plugin entry file

661eda9

activatePlugin: Simplify logic to find plugin entry file

3cbea6a

activatePlugin: Clarify accepted value for pluginPath option (absolut…

4487c3a

…e or relative path)

Merge branch 'trunk' into remove-dom-parser-from-blueprints

6b0f60f

Clean up after merge from trunk

1671b88

Format

3526f57

adamziel reviewed May 31, 2023

View reviewed changes

packages/playground/blueprints/src/lib/steps/activate-plugin.ts Outdated Show resolved Hide resolved

eliot-akira added 2 commits June 1, 2023 00:05

activatePlugin: Use error code for entry file not found; Rethrow any …

6f81a34

…PHP error

Format

977230f

adamziel added [Type] Enhancement New feature or request [Feature] PHP.wasm [Aspect] Node.js labels May 31, 2023

adamziel approved these changes May 31, 2023

View reviewed changes

adamziel merged commit 069930c into WordPress:trunk May 31, 2023
1 check passed

adamziel mentioned this pull request Jun 1, 2023

Blueprints: Support zipped assets without top-level folder #491

Merged

eliot-akira mentioned this pull request Jul 15, 2023

Blueprints: Isomorphic DOM - DOMParser, FormData, fetch #379

Closed

adamziel mentioned this pull request Oct 3, 2023

wp-now: ReferenceError: DOMParser is not defined while installing the plugin WordPress/playground-tools#113

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove DOMParser from Blueprints: installPlugin and installTheme #427

Remove DOMParser from Blueprints: installPlugin and installTheme #427

eliot-akira commented May 24, 2023 •

edited

eliot-akira commented May 24, 2023 •

edited

eliot-akira commented May 24, 2023 •

edited

eliot-akira commented May 24, 2023 •

edited

adamziel commented May 24, 2023

eliot-akira commented May 24, 2023 •

edited

adamziel May 31, 2023 •

edited

adamziel May 31, 2023 •

edited

eliot-akira May 31, 2023 •

edited

adamziel May 31, 2023

adamziel May 31, 2023

adamziel commented May 31, 2023

adamziel left a comment

eliot-akira commented May 31, 2023 •

edited

Remove DOMParser from Blueprints: installPlugin and installTheme #427

Remove DOMParser from Blueprints: installPlugin and installTheme #427

Conversation

eliot-akira commented May 24, 2023 • edited

What?

Why?

How?

Testing Instructions

eliot-akira commented May 24, 2023 • edited

eliot-akira commented May 24, 2023 • edited

eliot-akira commented May 24, 2023 • edited

adamziel commented May 24, 2023

eliot-akira commented May 24, 2023 • edited

adamziel May 31, 2023 • edited

Choose a reason for hiding this comment

adamziel May 31, 2023 • edited

Choose a reason for hiding this comment

eliot-akira May 31, 2023 • edited

Choose a reason for hiding this comment

adamziel May 31, 2023

Choose a reason for hiding this comment

adamziel May 31, 2023

Choose a reason for hiding this comment

adamziel commented May 31, 2023

adamziel left a comment

Choose a reason for hiding this comment

eliot-akira commented May 31, 2023 • edited

eliot-akira commented May 24, 2023 •

edited

eliot-akira commented May 24, 2023 •

edited

eliot-akira commented May 24, 2023 •

edited

eliot-akira commented May 24, 2023 •

edited

eliot-akira commented May 24, 2023 •

edited

adamziel May 31, 2023 •

edited

adamziel May 31, 2023 •

edited

eliot-akira May 31, 2023 •

edited

eliot-akira commented May 31, 2023 •

edited