Add lightweight saved-site export API - #4219
Conversation
bee906f to
8cb9ca4
Compare
a39cc68 to
4b0b60f
Compare
4b0b60f to
652e0af
Compare
8cb9ca4 to
3912eb4
Compare
3912eb4 to
fe66b7e
Compare
652e0af to
7cbfcdf
Compare
7cbfcdf to
314fd8f
Compare
| output: { | ||
| manualChunks: (id) => { | ||
| // Rollup recursively assigns the API entry's dependencies to this chunk. | ||
| // This prevents the optional Blueprint editor chunk from claiming OPFS storage. |
There was a problem hiding this comment.
This results in ~70% measured savings
Cold api.html startup With rule Without rule Savings
━━━━━━━━━━━━━━━━━━━━━━━ ━━━━━━━━━━━ ━━━━━━━━━━━━━━ ━━━━━━━━━━━━━━━━━━━━━━
Raw assets 706,309 B 2,212,465 B 1,506,156 B (68.08%)
─────────────────────── ─────────── ────────────── ──────────────────────
gzip assets 174,286 B 656,404 B 482,118 B (73.45%)
─────────────────────── ─────────── ────────────── ──────────────────────
Requests 3 11 8 (72.73%)
─────────────────────── ─────────── ────────────── ──────────────────────
HTML gzip 285 B 420 B 135 B
There was a problem hiding this comment.
This comment still doesn't make sense to me. I definitely could be missing something, but why would the Blueprint editor chunk claiming OPFS storage be an issue for the API entry point?
It might be that the build may then think the API entry point needs to import the Blueprint editor chunk in order to gain access to the OPFS-related modules. But that sounds like a build bug to me. Shouldn't we be able to build a separate set of chunks to serve the api.html entry point and completely disentangle from the larger Playground website builds?
There is something we don't understand well enough here, and I think it is worth understanding this in order to fix it.
There was a problem hiding this comment.
As a philosophical side note, if we have evidence to justify a particular line of code, it would be good to include that evidence in an inline comment so it continues to live and justify the code. Otherwise, it will be lost to GitHub history.
In this case, I think we probably should try to fix the build so this config hack is not needed, but if it turns out that Vite and rollup force us into this hack, we should explain it better so everyone truly can understand it.
One cool thing @adamziel mentioned the other day is asking agents to document things in language that is suitable to a junior developer. That's often great language for all of us, junior or not.
There was a problem hiding this comment.
@brandonpayton Yep, pretty much what you said. The chunking done by the build process end up such that a larger chunk gets loaded for what could have been a smaller chunk ideally. I am not sure to call that a build bug or just that the build process isn't super smart to figure the right chunking in every case. I will try to rephrase the doc to make it super easy for a junior dev to understand. 👍
| it('loads api.html and returns its API client', async () => { | ||
| const api = createAPIClient(); | ||
| mocks.consumeAPI.mockReturnValue(api); | ||
| const iframe = createIframe(); | ||
|
|
||
| await expect( | ||
| startPlaygroundAPI({ | ||
| iframe, | ||
| apiUrl: 'http://localhost/api.html', | ||
| }) | ||
| ).resolves.toBe(api); | ||
|
|
||
| expect(iframe.src).toBe('http://localhost/api.html'); | ||
| expect(mocks.consumeAPI).toHaveBeenCalledWith( | ||
| iframe.contentWindow, | ||
| iframe.ownerDocument!.defaultView | ||
| ); | ||
| expect(api.isConnected).toHaveBeenCalledTimes(1); | ||
| expect(api.isReady).toHaveBeenCalledTimes(1); | ||
| }); |
There was a problem hiding this comment.
I'm not sure this test adds enough value to justify its existence. It is also asserting on internals involving consumeAPI(). It's kind of like asserting that startPlaygroundAPI() has an implementation.
There was a problem hiding this comment.
L155-156 are meh, but the assertions above it are useful, I believe? So, I didn't bother removing those two meh assertions, cuz it's fine to over do it a little.
There was a problem hiding this comment.
Note to self: re-evaluate the test
|
|
||
| ## Saved-site export API | ||
|
|
||
| Use `startPlaygroundAPI()` to work with saved OPFS Playgrounds without booting WordPress, PHP, workers, or a service worker. The API endpoint must have the same origin and browser storage partition as the Playground that saved the site. WebKit requires save and export to use the same top-level origin, including its scheme, host, and port; a site saved while Playground is top-level is not visible to an API iframe embedded under another top-level origin. |
There was a problem hiding this comment.
Maybe we should omit "OPFS" here. It's an implementation detail. The API's purpose is to provide access to saved Playgrounds.
Actually... with that in mind, what if we rename api.html to something more specific like export-playground.html? This isn't being created because we want to launch a large API surface that serves different purposes.
There was a problem hiding this comment.
Maybe we should omit "OPFS" here. It's an implementation detail. The API's purpose is to provide access to saved Playgrounds.
We can have other saved playground sites that are not OPFS based and this API doesn't work with those, so perhaps its better to be upfront and accurate about it, even if that's an implementation detail. We are also not striving towards eventually covering all playground sites. In fact, it may not even be possible in all contexts.
Actually... with that in mind, what if we rename api.html to something more specific like export-playground.html? This isn't being created because we want to launch a large API surface that serves different purposes.
I earlier started with bridge.html but @adamziel suggested to use api.html since it can be expanded for other stuff in the future.
There was a problem hiding this comment.
Note to self: Rephrase highlighting how we are thinking and future potential
| expect(mocks.exportSavedSiteAsZip).toHaveBeenCalledWith('my-site', { | ||
| excludePatterns: ['/*', '!/wp-content/**'], | ||
| }); | ||
| expect(mocks.setAPIReady).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
How useful is it to assert that the implementation has exposeAPI() details?
There was a problem hiding this comment.
Maybe there is a reason we don't have them here, and maybe we talked about it yesterday, but I think an E2E test would be more useful for evidence that everything is wired properly (or sufficiently).
There was a problem hiding this comment.
Agree on the E2E test, I haven't added that yet. But the meh asserts seem fine to keep in an AI-first world. What seems very less useful for humans could be a meaningful signal for AI as it parses text/logs to build context?
There was a problem hiding this comment.
Note to self: Re evaluate the test
| output: { | ||
| manualChunks: (id) => { | ||
| // Rollup recursively assigns the API entry's dependencies to this chunk. | ||
| // This prevents the optional Blueprint editor chunk from claiming OPFS storage. |
There was a problem hiding this comment.
This comment still doesn't make sense to me. I definitely could be missing something, but why would the Blueprint editor chunk claiming OPFS storage be an issue for the API entry point?
It might be that the build may then think the API entry point needs to import the Blueprint editor chunk in order to gain access to the OPFS-related modules. But that sounds like a build bug to me. Shouldn't we be able to build a separate set of chunks to serve the api.html entry point and completely disentangle from the larger Playground website builds?
There is something we don't understand well enough here, and I think it is worth understanding this in order to fix it.
| output: { | ||
| manualChunks: (id) => { | ||
| // Rollup recursively assigns the API entry's dependencies to this chunk. | ||
| // This prevents the optional Blueprint editor chunk from claiming OPFS storage. |
There was a problem hiding this comment.
As a philosophical side note, if we have evidence to justify a particular line of code, it would be good to include that evidence in an inline comment so it continues to live and justify the code. Otherwise, it will be lost to GitHub history.
In this case, I think we probably should try to fix the build so this config hack is not needed, but if it turns out that Vite and rollup force us into this hack, we should explain it better so everyone truly can understand it.
One cool thing @adamziel mentioned the other day is asking agents to document things in language that is suitable to a junior developer. That's often great language for all of us, junior or not.
Co-authored-by: Brandon Payton <brandon@happycode.net>
## What Adds saved OPFS site ZIP exports with optional gitignore-style exclusion patterns, subtree pruning, stable directory permissions, and focused coverage. ## Why This establishes the storage-layer capability independently of the hosted API, so it can be reviewed and tested without client, routing, or deployment changes. ## Stack - 1 of 3 - Base: `trunk` - Next: #4219 ## Checks - `npm exec nx test playground-website` - `npm exec nx lint playground-website` - `npm exec nx typecheck playground-website` - `npm exec nx build playground-website`
## What Adds a short explanation to `api.html` for people who open the endpoint directly. The paragraph starts with the HTML `hidden` attribute and a small synchronous script reveals it only when `window.self === window.top`. The existing module import remains in `api.html`, but `bootPlaygroundAPI()` is called only when the page is embedded, so direct navigation does not initialize the export API and iframe consumers never receive a visible artifact. ## Direct navigation Opening `api.html` as the top-level page shows the intended explanation.  ## Embedded export For verification, the API iframe was deliberately made visible and given a border. It remained blank before and after exporting a real saved site.  ## `startPlaygroundWeb()` handling Passing `api.html` as the `remoteUrl` to `startPlaygroundWeb()` does not load this page. The client validates the URL before assigning `iframe.src` and throws an `Invalid remote URL` error because `startPlaygroundWeb()` requires the pathname to be `/remote.html`. The new direct-navigation explanation therefore covers someone opening `api.html` in a browser, while the existing client error covers this API misuse. ## Tested - Navigated directly to `api.html` and confirmed the explanation is visible, `window.playgroundAPI` is not initialized, and no page errors occur. - Loaded `api.html` in an iframe and confirmed its body remains visually empty, its paragraph retains `hidden` with zero client rects, `window.playgroundAPI` is initialized, and no page errors occur. - Called `startPlaygroundAPI()` with a deliberately visible iframe, seeded a real OPFS saved site, and ran `exportSavedSiteAsZip()`. - Confirmed the export returned `application/zip`, 647 bytes, with the ZIP `PK\x03\x04` signature. - Confirmed the iframe body remained empty and the paragraph had zero visible client rects both before and after export. - Confirmed `npm exec nx lint playground-website` and `npm exec nx typecheck playground-website` pass.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a lightweight in-origin /api.html endpoint and client API to export saved OPFS sites as ZIPs without booting the full Playground runtime.
Changes:
- Add
/api.htmlentry +bootPlaygroundAPI()runtime that exposes saved-site ZIP export viaexposeAPI. - Extend OPFS site storage ZIP export to support ordered gitignore-style exclusion patterns and preserve directory permissions in ZIPs.
- Add
startPlaygroundAPI()to@wp-playground/clientand ensure Vite chunking keeps export dependencies separate from the optional Blueprint editor.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/playground/website/vite.config.ts | Serves /api.html at root in dev and adds a separate Rollup chunk for the API boot path. |
| packages/playground/website/src/lib/state/opfs/opfs-site-storage.ts | Adds exclusion patterns and directory permission metadata when exporting a saved site as ZIP. |
| packages/playground/website/src/lib/state/opfs/opfs-site-storage.spec.ts | Extends tests for exclusion patterns and directory permission bits in ZIP output. |
| packages/playground/website/src/lib/boot-playground-api.ts | Adds an API bootstrap that exposes the ZIP export method over the universal API bridge. |
| packages/playground/website/src/lib/boot-playground-api.spec.ts | Tests that the API is exposed/forwarded and errors when OPFS storage is unavailable. |
| packages/playground/website/api.html | Adds a lightweight API entry page used for exports (intended for iframe embedding). |
| packages/playground/client/src/index.ts | Adds startPlaygroundAPI(), public API client/types, and shared URL validation logic. |
| packages/playground/client/src/index.spec.ts | Adds tests covering startPlaygroundAPI() URL behavior and API consumption. |
| packages/playground/client/README.md | Documents the saved-site export API usage and exclusion patterns semantics. |
| package.json | Adds the ignore dependency used for gitignore-style exclusions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… agent/pr4219-test-quality # Conflicts: # packages/playground/website/src/lib/state/opfs/opfs-site-storage.spec.ts
## What Configures `/api.html` as a network-first, no-store entry document and excludes the API HTML and hashed entry chunk from the website eager offline manifest. It covers both legacy `.htaccess` hosting and the WP Cloud redirect layer. ## Why A cached API entry can reference assets removed by a later deployment, while eagerly caching this external-only endpoint would add unnecessary website offline payload. The entry needs the same freshness rules as other deployment-sensitive documents. ## Stack - 3 of 3 - Base: #4219 - Previous: #4219 ## Checks - `npm exec nx test playground-remote` - `npm exec nx lint playground-remote` - `npm exec nx typecheck playground-remote` - `npm exec nx build playground-website` - PHP deployment harness executed with repository PHP.wasm: all tests passed - Built offline manifest checked to exclude `/api.html` and `/assets/api-*.js` --------- Co-authored-by: Brandon Payton <brandon@happycode.net>
What
Adds
startPlaygroundAPI(), the public saved-site export types, a lightweight/api.htmlruntime, API forwarding coverage, and Vite chunking that keeps the OPFS export path separate from the optional Blueprint editor.Why
Consumers need to export an existing OPFS Playground without booting WordPress, PHP, workers, or a service worker. The endpoint must share the origin and browser storage partition where the site was saved; WebKit additionally requires the same top-level origin.
Stack
Checks
npm exec nx test playground-clientnpm exec nx test playground-websitenpm exec nx lint playground-clientnpm exec nx lint playground-websitenpm exec nx typecheck playground-clientnpm exec nx typecheck playground-websitenpm exec nx build playground-clientnpm exec nx build playground-website