-
Notifications
You must be signed in to change notification settings - Fork 50
Studio: Implement studio site stop CLI command #2140
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
base: trunk
Are you sure you want to change the base?
Studio: Implement studio site stop CLI command #2140
Conversation
📊 Performance Test ResultsComparing 3cb5785 vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
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.
This is great overall 👍 My only issues with the implementation were that sendStopMessage was unnecessary, and that we shouldn't use the as keyword for any of the types from cli/lib/types/wordpress-server-ipc.ts. I went ahead and pushed a fix for both of those issues.
There's one more thing I think we should look into before landing this, and that is stopping the proxy server if no remaining running sites depend on it.
cli/commands/site/stop.ts
Outdated
| const appdata = await readAppdata(); | ||
| const site = appdata.sites.find( ( s ) => arePathsEqual( s.path, siteFolder ) ); |
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.
Let's use getSiteByFolder here once #2134 has landed.
| builder: ( yargs ) => { | ||
| return yargs; | ||
| }, |
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.
| builder: ( yargs ) => { | |
| return yargs; | |
| }, |
e8bc36f to
13095d4
Compare
|
@fredrikekelund thanks for the review! I've updated the code to stop the proxy if no longer needed. |
Related issues
Proposed Changes
studio site stopCLI command for gracefully stopping local WordPress siteslatestCliPidfrom appdataTesting Instructions
npm run cli:buildnode dist/cli/main.js site create --path pathnode dist/cli/main.js site stop --path pathlatestCliPidis added on start and removed when site is stoppedPre-merge Checklist