Skip to content

Try to stop server if site initialization failed #565

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.

Sign up for GitHub

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

Merged
fluiddot merged 2 commits into from
Oct 1, 2024
Merged

Try to stop server if site initialization failed #565

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f90a8af
Try to stop server if site initialization failed
fluiddot Sep 26, 2024
8f01dc1
Fix unit tests
fluiddot Sep 30, 2024
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/components/tests/header.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ describe( 'Header', () => {
startServer: jest.fn( () => {
throw new Error( 'Failed to start the server' );
} ),
stopServer: jest.fn( () => Promise.resolve( { running: false } ) ),
} );
render(
<SiteDetailsProvider>
Expand Down
1 change: 1 addition & 0 deletions src/hooks/use-site-details.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,7 @@ export function SiteDetailsProvider( { children }: SiteDetailsProviderProps ) {
"Please verify your site's local path directory contains the standard WordPress installation files and try again. If this problem persists, please contact support."
),

@sejas sejas Oct 1, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about including error.message to the alert?
I feel giving some tips to the user to fix the site will help the overall UX.

Something like:
Screenshot 2024-10-01 at 13 12 33

@fluiddot fluiddot Oct 1, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good point. This could be helpful when the user contacts support and we need to debug a failure.

@wojtekn wojtekn Oct 1, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we remove the underlying error, e.g., 'Error invoking remote method [...]'?

@fluiddot fluiddot Oct 1, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about removing it as it's part of the error message, it could be helpful when debugging issues in the future. What we could do is adding a title to that part conveying that's the raw error message, so users don't get confused about the content.
} );
getIpcApi().stopServer( id );
}

if ( updatedSite ) {
Expand Down
2 changes: 1 addition & 1 deletion src/lib/site-server-process-child.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ async function start() {
}

async function stop() {
await server.stopServer();
await server?.stopServer();
}

async function runPhp( data: unknown ) {
Expand Down