-
Notifications
You must be signed in to change notification settings - Fork 41
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
WP-NOW: Use rotatePHPRuntime to avoid memory error #152
Conversation
}; | ||
|
||
const phpInstances = []; | ||
const loadRuntime = async () => | ||
await NodePHP.loadRuntime(options.phpVersion); |
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.
Property 'loadRuntime' does not exist on type 'typeof NodePHP'.
This GitHub message looks suspicious, otherwise the PR seems to look good.
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.
I think the GitHub message was because we weren't using the latest version of Playground dependencies. Now it's gone.
@brandonpayton this could be interesting for you, and it would also make |
@adamziel, that sounds good! I can take a look. |
@adamziel , @bgrgicak , Now that we updated and adapted the Playground dependencies to the latest version, I could finish this PR. Would you mind reviewing it? cc: @brandonpayton |
packages/wp-now/src/wp-now.ts
Outdated
await rotatePHPRuntime({ | ||
php, | ||
recreateRuntime: loadRuntime, | ||
maxRequests: 400, |
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.
Maybe let's try 100
? Also, it would be nice to detect out of memory errors and auto-rotate – not that it's a blocker here.
maxRequests: 400, | |
maxRequests: 100, |
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!
Reverts #152 The php instance rotates correctly, but the new php instance doesn't have any folder mounted. It's a clean php. Reverting for now, and I'll create another PR. After rotating I get this result: `404 File not found`, and I confirmed the document root folder doesn't exist in VFS and therefore the WordPress files. <img width="518" alt="Captura de pantalla 2024-03-22 a las 12 52 35" src="https://github.com/WordPress/playground-tools/assets/779993/7242e364-cbb8-48a7-9e39-a703b49bba26"> cc: @adamziel
@adamziel , I reverted this PR. As it seems it was returning a |
…dPress#152) Migrates Playground from regexp-based [wp-sqlite-db](aaemnnosttv/wp-sqlite-db#55) to the official [sqlite-database-integration](https://github.com/WordPress/sqlite-database-integration) plugin which [translates queries using SQL parser](WordPress/sqlite-database-integration#9) instead of regular expressions. This makes for a more reliable SQLite integration that passes almost all WordPress unit tests. [Learn more](aaemnnosttv/wp-sqlite-db#55) about the problem with regular expressions.
What?
It uses the new function
rotatePHPRuntime
to mitigate the memory error.How?
Update the dependencies and use
rotatePHPRuntime
for the main instance.Testing Instructions
nvm use && npm install
npx nx build wp-now
node dist/packages/wp-now/cli.js start