Studio: Refactor Studio to depend on the CLI for starting sites#2172
Conversation
034011f to
e882445
Compare
e882445 to
d0e4359
Compare
fredrikekelund
left a comment
There was a problem hiding this comment.
Great work, @bcotrim! 👍 Very satisfying to see this come together. It's working nicely in my testing so far.
I left a couple of smaller comments.
I'd also be interested to hear your perspective of this site list --watch approach vs watching the config file for changes and using some mechanism (a version of the studio site status command, perhaps) to check the site status when latestCliPid changes. I don't really have a strong opinion there, but it would be good to have a documented reason for going one way or the other.
cli/commands/site/list.ts
Outdated
| choices: [ 'table', 'json' ], | ||
| default: 'table', | ||
| description: __( 'Output format' ), | ||
| } ) | ||
| .option( 'watch', { | ||
| type: 'boolean', | ||
| default: false, | ||
| description: __( 'Watch for site status changes and update the list in real-time' ), | ||
| } ); | ||
| }, | ||
| handler: async ( argv ) => { | ||
| try { | ||
| await runCommand( argv.format as 'table' | 'json' ); | ||
| await runCommand( argv.format as 'table' | 'json', argv.watch as boolean ); |
There was a problem hiding this comment.
choices: [ 'table', 'json' ] as const,
default: 'table' as const,
description: __( 'Output format' ),
} )
.option( 'watch', {
type: 'boolean',
default: false,
description: __( 'Watch for site status changes and update the list in real-time' ),
} );
},
handler: async ( argv ) => {
try {
await runCommand( argv.format, argv.watch );Kind of a nitpick, but that's how we get yargs to provide the correct types for the options.
| @@ -391,8 +392,8 @@ async function appBoot() { | |||
|
|
|||
| app.on( 'quit', () => { | |||
| void stopAllServersOnQuit(); | |||
There was a problem hiding this comment.
This will kill all running processes, regardless of whether they were started from the CLI or from the app. We could just accept this behavior, but @nightnei also had a pretty good idea to display a simple modal before quitting the app that says "Do you want to stop the running sites? Yes / Leave them runing"
There was a problem hiding this comment.
I like that idea! I would add a "remember my decision" checkbox. I think that provides enough tools for our users to adjust their experience as it work best for them. We can always revisit in the future.
Should I handle that change in this PR or as part of STU-1078 ?
| // CLI-managed sites don't have PHP instance, return cached theme details for Now | ||
| // ToDo: Implement logic to fetch theme details using mu-plugin? | ||
| if ( ! server.server.php ) { | ||
| return server.details.themeDetails; | ||
| } |
There was a problem hiding this comment.
Once we have WP-CLI support, we might also consider using the wp eval command
| export function executeCliCommand( | ||
| args: string[], | ||
| options: ExecuteCliCommandOptions = {} | ||
| ): CliCommandEventEmitter { |
There was a problem hiding this comment.
| ): CliCommandEventEmitter { | |
| ): [CliCommandEventEmitter, ChildProcess] { |
Optional, but we could return the child process instance from this function, too. It would save us from the dance of having to set the kill function on the EventEmitter
5a0ae52 to
dd0994d
Compare
fredrikekelund
left a comment
There was a problem hiding this comment.
It looks like the e2e/blueprints.test.ts tests are still failing, but I guess that's expected so long as we're in this intermittent state of using the Studio CLI to start sites, but still using Playground CLI to create sites.
Other than that, this looks good! 👍 I left a couple of comments. No huge blockers. The biggest one is probably to remove pm2ProcessMessageSchema in favor of childMessagePm2Schema
cli/logger.ts
Outdated
| /** | ||
| * Get the underlying ora spinner instance. | ||
| * Useful for sharing with other modules that need to update progress. | ||
| */ | ||
| public get spinner(): Ora { | ||
| return this._spinner; | ||
| } |
There was a problem hiding this comment.
This seems like a leftover, right? We could probably change the _spinner prop back to spinner
cli/lib/pm2-manager.ts
Outdated
| const bus = await getPm2Bus(); | ||
|
|
||
| const messageHandler = ( packet: unknown ) => { | ||
| const result = pm2ProcessMessageSchema.safeParse( packet ); |
There was a problem hiding this comment.
| const result = pm2ProcessMessageSchema.safeParse( packet ); | |
| const result = childMessagePm2Schema.safeParse( packet ); |
Let's reuse childMessagePm2Schema here. We can update the schema definition to also include data.process.name
| // Zod schema for PM2 process messages (IPC messages from child processes) | ||
| export const pm2ProcessMessageSchema = z.object( { | ||
| process: z.object( { | ||
| name: z.string(), | ||
| pm_id: z.number(), | ||
| } ), | ||
| raw: z | ||
| .object( { | ||
| topic: z.string(), | ||
| } ) | ||
| .passthrough(), | ||
| } ); | ||
| export type Pm2ProcessMessage = z.infer< typeof pm2ProcessMessageSchema >; |
There was a problem hiding this comment.
| // Zod schema for PM2 process messages (IPC messages from child processes) | |
| export const pm2ProcessMessageSchema = z.object( { | |
| process: z.object( { | |
| name: z.string(), | |
| pm_id: z.number(), | |
| } ), | |
| raw: z | |
| .object( { | |
| topic: z.string(), | |
| } ) | |
| .passthrough(), | |
| } ); | |
| export type Pm2ProcessMessage = z.infer< typeof pm2ProcessMessageSchema >; |
Let's remove this per my suggestion in cli/lib/pm2-manager.ts
| } ), | ||
| event: z.string(), | ||
| } ); | ||
| export type Pm2ProcessEvent = z.infer< typeof pm2ProcessEventSchema >; |
There was a problem hiding this comment.
| export type Pm2ProcessEvent = z.infer< typeof pm2ProcessEventSchema >; |
We're only using the schema, so let's remove the inferred type.
| @@ -391,8 +392,8 @@ async function appBoot() { | |||
|
|
|||
| app.on( 'quit', () => { | |||
| void stopAllServersOnQuit(); | |||
| }; | ||
|
|
||
| class CliCommandEventEmitter extends EventEmitter { | ||
| export class CliCommandEventEmitter extends EventEmitter { |
There was a problem hiding this comment.
| export class CliCommandEventEmitter extends EventEmitter { | |
| class CliCommandEventEmitter extends EventEmitter { |
No need to export this class, AFAICT
| * When true, file watchers should ignore site events to prevent interference | ||
| * with the ongoing operation. | ||
| */ | ||
| hasOngoingOperation = false; |
| wpVersion?: string; | ||
| blueprint?: unknown; | ||
| } | ||
| logger: Logger< string >, |
There was a problem hiding this comment.
I don't think we need to address this in the current PR, but the pattern of passing in a logger instance seems to confuse responsibilities. I suggest following up later (maybe after the project launch) by refactoring this file to export a WordPressServerManager class that extends EventEmitter, and emitting the appropriate events (progress, success, error) as part of the startup flow.
|
@fredrikekelund @nightnei thanks for the reviews!
Yes, I think it will be easier to address once we have the full implementation |
Related issues
Proposed Changes
--watchtostudio site listcreateCliServerProcessso Studio can start and stop sites using Studio CLITesting Instructions
npm installnpm startstudio site listreturns the correct status for the sitePre-merge Checklist