Skip to content
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

Improve logs and preview server behavior #404

Merged
merged 1 commit into from
Oct 22, 2024
Merged

Conversation

benjreinhart
Copy link
Contributor

When playing around with the app, I saw some vite dev server behavior I wasn't sure on and then the logs (somewhat lacking) made it more confusing to me. I wasn't sure if we had tried to re-boot it, or what. I also didn't see anything about it shutting down. Then I noticed some websocket events were firing multiple times.

This PR:

  1. Removes the duplicated websocket events
  2. Fixes a bug where we are replying to a specific connection rather than broadcasting during the lifetime of a running preview server. This has a couple issues:
    1. That connection may no longer exist (e.g., user refreshes page)
    2. This would only forward output to one client. If I have multiple tabs open, I wouldn't see the output in the other tab(s).
  3. Adds consistent logging for the preview server states (booting, running, stopping)
  4. Cleans the logs. Adds a "srcbook" source to differentiate from vite or other output.

Initial boot

Screenshot 2024-10-21 at 9 05 14 PM

Stopped the server which caused it to auto-restart

Screenshot 2024-10-21 at 9 05 43 PM

@benjreinhart benjreinhart requested a review from 1egoman October 22, 2024 04:11
@@ -50,39 +50,34 @@ async function previewStart(
const existingProcess = processMetadata.get(app.externalId);

if (existingProcess) {
conn.reply(`app:${app.externalId}`, 'preview:status', {
wss.broadcast(`app:${app.externalId}`, 'preview:status', {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realize this is sorta confusing, but the conn object represents one specific connection and thus conn.reply only replies to that one connected client, whereas wss.broadcast broadcasts to all clients connected over a given topic.

One of the problems this can cause here is that, since this is being held in a closure for the duration of a long-lived process, the connection may close long before the preview server is done running (refreshing the page will cause this). After the connection is closed, all preview server output would never make its way to any client.

cc @1egoman @nichochar

@benjreinhart benjreinhart merged commit 828a361 into main Oct 22, 2024
3 checks passed
@benjreinhart benjreinhart deleted the preview-server branch October 22, 2024 04:16
@@ -111,11 +105,12 @@ async function previewStart(
onExit: (code) => {
processMetadata.delete(app.externalId);

conn.reply(`app:${app.externalId}`, 'preview:status', {
console.log('HERE???', code);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, I'll remove this.

@@ -56,11 +56,11 @@ function Preview() {
case 'stopped':
return (
<div className="flex justify-center items-center w-full h-full">
{lastStoppedError === null ? (
<span className="text-tertiary-foreground">Stopped preview server.</span>
{exitCode === null || exitCode === 0 ? (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I improved this a bit here: e94f157

@nichochar
Copy link
Contributor

Thanks for all the touch ups!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants