Update studio site list CLI command - #2127
Conversation
| const loggerError = new LoggerError( __( 'Failed to start site infrastructure' ), error ); | ||
| logger.reportError( loggerError ); | ||
| } | ||
| process.exit( 1 ); |
There was a problem hiding this comment.
We already set the exit code in cli/logger.ts. No need to explicitly exit the process in this way.
|
@fredrikekelund after Maybe something is wrong with my environment. Could you please double-check, and if it indeed works for you, then I will take a look deeper into why it fails for me. |
📊 Performance Test ResultsComparing 27207ff vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
| site.status, | ||
| site.name, | ||
| site.path, | ||
| { href: new URL( site.url ).toString(), content: site.url }, |
There was a problem hiding this comment.
@nightnei, I believe the error you mentioned #2127 (comment) happens here. Could you please add a simple console.log(site.url) call right above the table.push call here to see what might be causing the error here?
There was a problem hiding this comment.
LGTM and everything works as expected:
However, I have a few questions:
- If Studio is running and we have started sites - they are not reflected in CLI, is it expected?
- After running a site via CLI, then running Studio (with previously started sites, so it's expected that they will be started after running Studio again) - I get the next error:
Marking as "requested changes", just not to miss your answer :)
It's expected for now. We intend to make the Studio CLI the "back-end" of Studio, and use it to start/stop/create sites. At that point, the CLI "running status" will be the single source of truth. For now, though, a site's running status isn't shared between the CLI and the app.
Same thing: this is expected for now, but it will be resolved when the Studio CLI is responsible for starting/stopping/creating sites. |
There was a problem hiding this comment.
Code LGTM and works as described 👍
Should we also display the WordPress version, or just PHP?
Edit: I just realized we don't store wp version in appdata, so we could address this in a follow-up.
We show this in Studio settings using common/lib/get-wordpress-version.ts
| return table; | ||
| } | ||
| for await ( const site of sites ) { | ||
| const isOnline = await isServerRunning( site.id ); |
There was a problem hiding this comment.
isServerRunning will eventually call listProcesses I think we could do some optimization here to only fetch the list of running processes once.
I pondered this and ultimately decided to remove the PHP version. There are three truly relevant properties that this command should return, IMO: running status, path, and URL. The name is largely irrelevant in the CLI context, but still makes sense to keep for consistency. Rather than printing more properties with this command (PHP version and WP version), users can turn to the
I implemented a |
| const cache = new Map< () => Promise< unknown >, { result: unknown; timestamp: number } >(); | ||
|
|
||
| export function cacheFunctionTTL< Args extends unknown[], Return >( | ||
| fn: ( ...args: Args ) => Promise< Return >, | ||
| ttl = 1 * 1000 | ||
| ) { | ||
| return async ( ...args: Args ) => { | ||
| const cachedValue = cache.get( fn ); | ||
| if ( cachedValue && Date.now() - cachedValue.timestamp < ttl ) { | ||
| return cachedValue.result as Return; | ||
| } | ||
| const value = await fn( ...args ); | ||
| cache.set( fn, { result: value, timestamp: Date.now() } ); | ||
| return value; | ||
| }; | ||
| } |
There was a problem hiding this comment.
| const cache = new Map< () => Promise< unknown >, { result: unknown; timestamp: number } >(); | |
| export function cacheFunctionTTL< Args extends unknown[], Return >( | |
| fn: ( ...args: Args ) => Promise< Return >, | |
| ttl = 1 * 1000 | |
| ) { | |
| return async ( ...args: Args ) => { | |
| const cachedValue = cache.get( fn ); | |
| if ( cachedValue && Date.now() - cachedValue.timestamp < ttl ) { | |
| return cachedValue.result as Return; | |
| } | |
| const value = await fn( ...args ); | |
| cache.set( fn, { result: value, timestamp: Date.now() } ); | |
| return value; | |
| }; | |
| } | |
| export function cacheFunctionTTL< Args extends unknown[], Return >( | |
| fn: ( ...args: Args ) => Promise< Return >, | |
| ttl = 1000 | |
| ) { | |
| const cache = new Map< string, { result: Return; timestamp: number } >(); | |
| return async ( ...args: Args ) => { | |
| const key = JSON.stringify( args ); | |
| const cachedValue = cache.get( key ); | |
| if ( cachedValue && Date.now() - cachedValue.timestamp < ttl ) { | |
| return cachedValue.result; | |
| } | |
| const value = await fn( ...args ); | |
| cache.set( key, { result: value, timestamp: Date.now() } ); | |
| return value; | |
| }; | |
| } |
Currently this function has a bug for calls with different arguments.
A test like this would fail:
it( 'should return different results for different arguments', async () => {
const fetchUser = jest.fn( async ( userId: number ) => {
return { id: userId, name: `User ${ userId }` };
} );
const cachedFetchUser = cacheFunctionTTL( fetchUser, 1000 );
const user1 = await cachedFetchUser( 1 );
const user2 = await cachedFetchUser( 2 );
expect( user1 ).toEqual( { id: 1, name: 'User 1' } );
expect( user2 ).toEqual( { id: 2, name: 'User 2' } );
} );There was a problem hiding this comment.
Good catch 👍 I've updated cacheFunctionTTL to account for function arguments, too (using fast-deep-equal to compare function arguments)
bcotrim
left a comment
bcotrim
left a comment
There was a problem hiding this comment.
Thanks for making the change to the cache function. I would still go for the simple suggestion but your implementation is very solid and well tested.
LGTM 👍
3 participants
Related issues
Proposed Changes
@youknowriad originally implemented the
studio site listcommand in #1742. This PR updates that command with relevant site details now that #2096 has been merged. We now include:Testing Instructions
npm run cli:buildnode dist/cli/main.js site listnode dist/cli/main.js site start --path PATH_TO_SITE(wherePATH_TO_SITEis the path to a Studio site)node dist/cli/main.js site listagainPre-merge Checklist