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

Remove DOMParser from Blueprints: installPlugin and installTheme #427

Merged

Conversation

eliot-akira
Copy link
Collaborator

@eliot-akira eliot-akira commented May 24, 2023

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:

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

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

@eliot-akira eliot-akira changed the title Remove DOMParser from Blueprints Draft: Remove DOMParser from Blueprints May 24, 2023
@eliot-akira
Copy link
Collaborator Author

eliot-akira commented May 24, 2023

@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
Copy link
Collaborator Author

eliot-akira commented May 24, 2023

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).

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

@eliot-akira
Copy link
Collaborator Author

eliot-akira commented May 24, 2023

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.

README.md Outdated Show resolved Hide resolved
@adamziel
Copy link
Collaborator

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

@eliot-akira
Copy link
Collaborator Author

eliot-akira commented May 24, 2023

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.

@eliot-akira eliot-akira changed the title Remove DOMParser from Blueprints Remove DOMParser from Blueprints: installPlugin and installTheme May 31, 2023
* 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 {
Copy link
Collaborator

@adamziel adamziel May 31, 2023

Choose a reason for hiding this comment

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

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 👍

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

@adamziel adamziel May 31, 2023

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@eliot-akira eliot-akira May 31, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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!

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

@adamziel
Copy link
Collaborator

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

Copy link
Collaborator

@adamziel adamziel left a comment

Choose a reason for hiding this comment

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

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

@adamziel adamziel merged commit 069930c into WordPress:trunk May 31, 2023
@eliot-akira
Copy link
Collaborator Author

eliot-akira commented May 31, 2023

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.

adamziel pushed a commit that referenced this pull request Jun 1, 2023
## 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

<!-- Please include step by step instructions on how to test this PR.
-->
1. Check out the branch.
2. Run `nx test playground-blueprints`
adamziel added a commit that referenced this pull request Jun 1, 2023
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.
adamziel added a commit that referenced this pull request Jun 1, 2023
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.
adamziel added a commit that referenced this pull request Jun 1, 2023
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.
Pookie717 added a commit to Pookie717/wordpress-playground that referenced this pull request Oct 1, 2023
## 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

<!-- Please include step by step instructions on how to test this PR.
-->
1. Check out the branch.
2. Run `nx test playground-blueprints`
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.

3 participants