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

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

Closed
eliot-akira opened this issue May 17, 2023 · 5 comments · Fixed by #1192
Closed

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

eliot-akira opened this issue May 17, 2023 · 5 comments · Fixed by #1192

Comments

@eliot-akira
Copy link
Collaborator

eliot-akira commented May 17, 2023

From this merged pull request:

@wp-playground/blueprints will need to be smart about providing a Node.js polyfill for new DOMParser() and fetch().

Currently, the following browser globals are found in the blueprints package.


Possible solution: Prepare a proxy module/package that exports native DOM API for the browser and jsdom for Node.js.

The closest NPM package I found was k0michi/isomorphic-dom, which uses the exports property in its package.json to define browser and server exports.

There's also fetch-shim which does the same for fetch and undici; however, this latter module is now in Node.js core as global fetch since v18 so it's no longer necessary. EDIT: Just learned that global FormData is also available since Node.js v18. So it's only DOMParser that needs to be polyfilled on the server side.

@adamziel
Copy link
Collaborator

Thank you, this would be great!

An alternative fix would be to remove reliance on DOMParser entirely, like what this PR did for the activatePlugin step: https://github.com/WordPress/wordpress-playground/pull/351/files

@eliot-akira
Copy link
Collaborator Author

eliot-akira commented May 23, 2023

OK, I see there are two diverging paths, depending on the long-term design of Playground Blueprints.

  • Remove use of asDOM helper, DOMParser, and any browser-specific DOM API (and types like HTMLFormElement)

    This implies that Blueprint steps, existing and those added in the future, should avoid DOM manipulations to perform their actions. That might be limiting, but the advantage is that the steps can run on browser and server side by only using available Playground API.

    I'm kind of leaning toward this approach, it seems simpler to maintain. If you agree, I can start on a pull request to attempt a rewrite of the installPlugin and installTheme steps.

  • Provide an isomorphic DOM library that exports native DOM API for the browser and jsdom for Node.js

    This way, Blueprint steps can perform DOM manipulations through this proxy library. They should take care to avoid using any other browser-specific API beyond what jsdom and Node.js provides. The advantage is more flexibility for future steps to be added - maybe there are certain steps that can only be accomplished via the DOM. The disadvantage is the complexity of pretending that there's a DOM on the server side, and possible inefficiency of jsdom library.

    I found in the PR you mentioned that there's a commit which modified the asDOM helper to dynamically import jsdom on the server side. ed28a2b#diff-2c904654899ecec0c61cfd1da4e48ed08c13010af043f16185cedb8e68b9f37dR3-R9

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

    export async function asDOM(response: PHPResponse) {
    	if (typeof process !== 'undefined' && process.release?.name === 'node') {
    		const { JSDOM } = await import('jsdom');
    		return new JSDOM(response.text).window.document;
    	} else {
    		return new DOMParser().parseFromString(response.text, 'text/html')!;
    	}
    }

    In packages/playground/client/vite.config.ts

    	rollupOptions: {
    		// External packages that should not be bundled into your library.
    		external: ['jsdom'],
    	},

    The simplest solution might be to bring back the above changes, then the installPlugin and installTheme steps would work as is on the server. And future steps could use the asDOM helper for any DOM manipulations.

    I briefly looked into creating a new package using nx generate, something like @wp-playground/isomorphic-dom. It seems quite involved, like figuring out how to provide separate exports for browser and server. I also question whether this is even necessary, if the DOM proxy would only be used in the blueprints package.

@adamziel
Copy link
Collaborator

I'm kind of leaning toward this approach, it seems simpler to maintain. If you agree, I can start on a pull request to attempt a rewrite of the installPlugin and installTheme steps.

Yes please @eliot-akira ! That sounds absolutely fantastic 💯

I briefly looked into creating a new package using nx generate, something like @wp-playground/isomorphic-dom. It seems quite involved, like figuring out how to provide separate exports for browser and server. I also question whether this is even necessary, if the DOM proxy would only be used in the blueprints package.

That's also my concern with isomorphic DOM. If Playground had some public API that exposed DOM functions that would be different, but right now DOM is an implementation detail and a burden to maintain.

DOM might become indispensable one day, and if it does we'll figure it out based on the specific use-case that comes up. For now, I really like the idea of avoiding the complexity associated with a custom isomorphic-dom package.

adamziel pushed a commit that referenced this issue May 31, 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:

- #379 

The initial idea was to introduce an "isomorphic DOM" library that
exports native DOM API for the browser and
[jsdom](https://github.com/jsdom/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
Copy link
Collaborator Author

eliot-akira commented Jul 15, 2023

Hi @sejas - I wanted to mention this issue concerning the use of DOMParser in Blueprint steps. I think it's related to the question of supporting all Blueprint steps in Node.js (and wp-now), and also about supporting Node.js version 18 which lacks the File and CustomEvent class.


The original topic of this issue was about "isomorphic DOM", using the jsdom library to polyfill DOMParser and other browser-specific classes, so that Blueprints can run on Node.js.

With Adam's feedback, the approach was changed to remove the use of all browser-only APIs in Blueprint steps.

Mainly that involved rewriting some steps to not use GET/POST request and DOMParser for interacting with the site, and instead call PHP functions directly to achieve the same result.

Apparently that was done for activatePlugin and activateTheme already, and I worked on a pull request for rewriting installPlugin and installTheme.

The remaining step that's yet to be rewritten is importFile.


While I was working on the PR, I discovered that jsdom has an incomplete polyfill for the File class, which is missing the method arrayBuffer(). That caused some tests to fail, so I had to wrap the class to provide the method.

This workaround requires that all Blueprint steps that use the File class must import it from steps/common, instead of assuming that it exists as a global name. That may be convenient for polyfilling the same class for Node.js 18.


Also relevant:

@adamziel
Copy link
Collaborator

The only step still using the DOM API is importFile. The arrayBuffer() method and File and Blob logic have been polyfilled in Node.js.

adamziel added a commit that referenced this issue Apr 3, 2024
…ress-Importer

Rewires the importFile step to:

* Be named importWxz. The old name continues to work, though.
* Use the humanmade/WordPress-Importer for importing content – it is
  more reliable than the official wordpress-importer and has a
  convenient PHP API.
* Drop WXZ support – it's a maintenance burden and doesn't add clear
  value.

Closes #1183
Closes #379
Closes #1158
Closes #1161
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants