-
Notifications
You must be signed in to change notification settings - Fork 57
Remove PHP-WASM dependencies from Studio #2281
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
Conversation
|
Let's remove the |
epeicher
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @bcotrim for this huge reduction in size 🙌
I have done some initial testing, and I have identified that creating a site does not start it for me. Also, when deleting it, an error is displayed, although the deletion completes. Please see a video below.
CleanShot.2026-01-08.at.17.08.23.mp4
Please let me know if you need the logs.
|
@epeicher |
@bcotrim, I have been investigating more, and I have done the following that fix my issues:
That fixes all the issues I was having, I am going to do some additional testing, but this looks good so far 🥳 |
epeicher
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again @bcotrim for these massive changes. I have done a round of testing, and these are my findings.
- Create a new site - verify it works - ✅
This works fine, I have identified a minor issue that I am not able to reproduce intrunkbut it is not a blocker, and it could be tackled as a follow-up if required. When creating a site, there is a moment where the list of sites displays the new one duplicated, please see screenshot:
- Start/stop sites - verify server operations work - ✅
- Export a site - verify backup works - ✅
- Import a site - verify restore works - ❌
Importing a recently exported backup fails with the errorDatabase import failed- Please find error logs here 3a0e9-pb - Run tests:
npm test- ✅ - Build installer:
npm run package- verify app.asar is ~295 MB - ✅ -294M Jan 9 13:15 app.asar(in trunk it occupies 1.4G 🤯 )
I have measured the time it takes to create a site (approximately), and I have observed that on my Mac, it takes around 16 seconds on trunk (using npm startand having a few sites) and it takes around 8 seconds on this branch, so this is a great improvement 🙌
All in all, I would like to have another pair of eyes to review the changes, but this is looking great, amazing work.
* Update studio-cli.bat so we can read from stdin * Don't silence chcp warnings
…remove-php-wasm-from-studio # Conflicts: # cli/commands/site/set.ts # cli/commands/site/tests/set.test.ts # cli/lib/run-wp-cli-command.ts # package-lock.json # src/hooks/tests/use-add-site.test.tsx # src/hooks/use-add-site.ts # src/modules/add-site/components/create-site-form.tsx # src/modules/add-site/index.tsx # src/modules/site-settings/edit-site-details.tsx # src/site-server.ts
|
@fredrikekelund @epeicher with most (all?) of other on-going changes to the |
|
#2404 is already huge, so I recommend we land that PR first and then merge this PR directly to I'm reviewing this PR now 👍 |
fredrikekelund
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this LGTM 👍 Nice work, @bcotrim!
The one thing we should look into before merging is the function for checking the current WP-CLI version. We'll need a different approach there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use this function in scripts/download-node-binary.mjs, too?
In the future, we might consider also targeting src/lib/import-export/import/handlers/backup-handler-zip.ts, but that's for another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
common/types/blueprint.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't we import these directly from @wp-playground/blueprints?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use the extractZip function from common here?
| /** | ||
| * Get a temporary path for tests | ||
| */ | ||
| function getTmpPath( subfolder: string ): string { | ||
| return path.join( os.tmpdir(), `studio-tests-${ subfolder }` ); | ||
| } | ||
|
|
||
| /** | ||
| * Get the base path for server files (WordPress versions, SQLite, etc.) | ||
| */ | ||
| function getBasePath(): string { | ||
| if ( process.env.NODE_ENV === 'test' ) { | ||
| return getTmpPath( 'server-files' ); | ||
| } | ||
| return getServerFilesPath(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The big reason for having this extra layer on top of src/storage/paths is that we add getTmpPath, right? How necessary is that?
src/lib/wordpress-server-types.ts
Outdated
| }; | ||
| start( siteId?: string ): Promise< void >; | ||
| stop(): Promise< void >; | ||
| runPhp( data: { code: string; [ key: string ]: unknown } ): Promise< string >; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| runPhp( data: { code: string; [ key: string ]: unknown } ): Promise< string >; |
Can't we just remove the runPhp method now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that we now have just a single class implementing this interface, I'm wondering if we really need it..?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking forward to the day when we've consolidated this type of functionality in the CLI…
| async function getWPCliVersionFromInstallation(): Promise< string > { | ||
| return new Promise( ( resolve ) => { | ||
| const [ emitter ] = executeCliCommand( [ 'wp', '--path', getWpCliFolderPath(), '--version' ], { | ||
| output: 'capture', | ||
| } ); | ||
|
|
||
| emitter.on( 'success', ( { result } ) => { | ||
| const stdout = result?.stdout || ''; | ||
| if ( stdout.startsWith( 'WP-CLI ' ) ) { | ||
| const version = stdout.split( ' ' )[ 1 ]; | ||
| resolve( version ? `v${ version }` : '' ); | ||
| } else { | ||
| resolve( '' ); | ||
| } | ||
| } ); | ||
|
|
||
| emitter.on( 'failure', () => resolve( '' ) ); | ||
| emitter.on( 'error', () => resolve( '' ) ); | ||
| } ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work. The --path argument is parsed by the Studio CLI and must point to a site directory.
Off the top of my head, the best way to resolve this might be to add a hidden --no-path option to the wp command. Thoughts, @bcotrim?
|
@bcotrim "Size reduction:" is a bit misleading as it compares cleanup with the cli-i2 branch. 1.6.8 is ~1.8 GB, so there is no reduction when compared to base brach. |
This was our original goal. The fact that we've added a new bundled node runtime since @bcotrim opened this PR will impact the installer size, and we expect it to be slightly larger than 1.6.8. But yeah, even if the PR description might lead one to believe that this change reduces the installer filesize, we are really just keeping it in check 👍 |
|
@fredrikekelund, it's okay. I just wanted to clarify this to ensure the reader doesn't think this cleanup provides size reduction for the app. |
fredrikekelund
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 There are still a couple of comments from my previous review that I'd love to discuss, but they're no blockers.
Let's move ahead with landing this
|
For the record, the reason I made the |
Thanks for pointing it out! I Removed it, since it no longer makes sense |
📊 Performance Test ResultsComparing 7111f6a vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
Related issues
Proposed Changes
Remove PHP-WASM packages (~1.1 GB) from the desktop app's ASAR bundle. The desktop app now uses CLI for all WordPress operations, making these dependencies redundant.
common/types/php-versions.ts) to replace@php-wasm/universalimportssrc/lib/wordpress-provider/)getProviderConstantsIPC/Redux layer - components import directly fromcommon/constantssrc/lib/download-utils.tsvendor/wp-now/directoryforge.config.tsignore patterns to exclude PHP-WASM from bundleTesting Instructions
npm installnpm startnpm testnpm run package- verify app.asar is ~295 MBPre-merge Checklist