-
Notifications
You must be signed in to change notification settings - Fork 53
Sync site list when CLI creates sites while app is running #2313
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: dev/studio-cli-i2
Are you sure you want to change the base?
Sync site list when CLI creates sites while app is running #2313
Conversation
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.
Test STU-1164 (Site creation)
Worked as expected
- Run node dist/cli/main.js site set-domain hello.wp.local --path PATH_TO_SITE
The changes are not reflected in Studio, tested a few times and reinstalled node_moduels and rebuild
set-php-version
Also changes are not reflected in Studio
set-wp-version
Stucks endlessly on Changing WordPress version… and can't be termninated with controll+c
UPDATE: I got error and again it stucks:

|
thanks for the review @nightnei I couldn't replicate any of the issues, can you run |
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 and works as expected 👍
Looks like something was wrong locally for me when I tested before
sejas
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.
We removed and introduced the useIpcListener( 'user-data-updated' listener multiple times in the past. See:
I understand this is a good way to update the Studio UI from external changes, such as the CLI creating a new site, although I wonder why we removed it in the first place.
sejas
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.
It works as expected. Great work! I confirmed that the site appears automatically in the left sidebar when creating a new site with the Studio CLI. I also tested node dist/cli/main.js site set-domain hello.wp.local --path /tmp/test-site-cli and node dist/cli/main.js site set-php-version 8.4 --path /tmp/test-site-cli.
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.
In #2172, we decided that Studio should rely on studio site list --watch to subscribe to updates to the list of sites. The logic in src/modules/cli/lib/execute-site-watch-command.ts already supports parsing new sites, AFAICT. The reason new sites don't show up on the front-end is most likely that the site-status-changed IPC event handler only supports sites that already exists.
Instead of implementing this second, competing approach for subscribing to changes, we should look into fixing the site-status-changed IPC event handler.
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 a concern for a later PR, but I think we should reconsider storing a global list of SiteServer instances in the node process. The CLI (or pm2, really) will soon be the source of truth for whether a server is running. It might make more sense to treat server instances as ephemeral variables in the node process, always reading from disk or the CLI whenever the client process queries them.
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.
@bcotrim and I just huddled about this change and concluded that while we are both a little concerned about the relationship between the appdata watcher and the site list watcher being confusing, we still think it's a good idea to land this change for now, to resolve the concrete issues at hand and keep up the momentum.
@bcotrim will add a pretty exhaustive comment for now as a first step towards clarifying the relationship between the newly added watcher callback and the site list watcher (src/modules/cli/lib/execute-site-watch-command.ts). He'll also add a Linear issue detailing potential future refactorings in this area, to ensure the architecture remains clear and quality doesn't deteriorate with future changes
|
This is up to you, @bcotrim, but I still also think we could consider updating the |







Related issues
Proposed Changes
useIpcListenerforuser-data-updatedevent inuse-site-details.tsxto refresh the site list when the appdata file changes externally (e.g., when CLI creates a site or modifies site settings)Root cause: The file watcher (
user-data-watcher.ts) already broadcastsuser-data-updatedIPC events when the appdata file changes, butuse-site-details.tsxwas not listening to this event. The snapshot slice listened to it for preview sites, but the main site list did not.Testing Instructions
Test STU-1164 (Site creation)
npm run cli:buildnode dist/cli/main.js site create --path /tmp/test-site-cliTest STU-1163 (Site settings changes)
node dist/cli/main.js site set-domain hello.wp.local --path PATH_TO_SITEset-php-version,set-wp-version,set-httpsPre-merge Checklist