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.

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

Improve logs and preview server behavior #404

Merged
merged 1 commit into from
Oct 22, 2024
Merged

Improve logs and preview server behavior #404

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 15 additions & 46 deletions packages/api/server/channels/app.mts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ const processMetadata = new Map<string, ProcessMetadata>();
async function previewStart(
_payload: PreviewStartPayloadType,
context: AppContextType,
conn: ConnectionContextType,
wss: WebSocketServer,
) {
const app = await loadApp(context.params.appId);

Expand All @@ -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

status: 'running',
url: `http://localhost:${existingProcess.port}/`,
});
return;
}

conn.reply(`app:${app.externalId}`, 'preview:status', {
wss.broadcast(`app:${app.externalId}`, 'preview:status', {
url: null,
status: 'booting',
});

const onChangePort = (newPort: number) => {
processMetadata.set(app.externalId, { process, port: newPort });
conn.reply(`app:${app.externalId}`, 'preview:status', {
wss.broadcast(`app:${app.externalId}`, 'preview:status', {
url: `http://localhost:${newPort}/`,
status: 'running',
});
};

// NOTE: Buffering all logs into an array like this might be a bad idea given this could grow in
// an unbounded fashion.
let bufferedLogs: Array<String> = [];

const process = vite({
args: [],
cwd: pathToApp(app.externalId),
stdout: (data) => {
const encodedData = data.toString('utf8');
console.log(encodedData);
bufferedLogs.push(encodedData);

conn.reply(`app:${app.externalId}`, 'preview:log', {
wss.broadcast(`app:${app.externalId}`, 'preview:log', {
log: {
type: 'stdout',
data: encodedData,
Expand All @@ -99,9 +94,8 @@ async function previewStart(
stderr: (data) => {
const encodedData = data.toString('utf8');
console.error(encodedData);
bufferedLogs.push(encodedData);

conn.reply(`app:${app.externalId}`, 'preview:log', {
wss.broadcast(`app:${app.externalId}`, 'preview:log', {
log: {
type: 'stderr',
data: encodedData,
Expand All @@ -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.


wss.broadcast(`app:${app.externalId}`, 'preview:status', {
url: null,
status: 'stopped',
stoppedSuccessfully: code === 0,
logs: code !== 0 ? bufferedLogs.join('\n') : null,
code: code,
});
},
});
Expand All @@ -140,20 +135,12 @@ async function previewStop(
conn.reply(`app:${app.externalId}`, 'preview:status', {
url: null,
status: 'stopped',
stoppedSuccessfully: true,
logs: null,
code: null,
});
return;
}

result.process.kill('SIGTERM');

conn.reply(`app:${app.externalId}`, 'preview:status', {
url: null,
status: 'stopped',
stoppedSuccessfully: true,
logs: null,
});
}

async function dependenciesInstall(
Expand Down Expand Up @@ -247,30 +234,12 @@ async function onFileUpdated(payload: FileUpdatedPayloadType, context: AppContex
export function register(wss: WebSocketServer) {
wss
.channel('app:<appId>')
.on('preview:start', PreviewStartPayloadSchema, previewStart)
.on('preview:start', PreviewStartPayloadSchema, (payload, context) =>
previewStart(payload, context, wss),
)
.on('preview:stop', PreviewStopPayloadSchema, previewStop)
.on('deps:install', DepsInstallPayloadSchema, dependenciesInstall)
.on('deps:clear', DepsInstallPayloadSchema, clearNodeModules)
.on('deps:status', DepsStatusPayloadSchema, dependenciesStatus)
.on('file:updated', FileUpdatedPayloadSchema, onFileUpdated)
.onJoin(async (topic, ws) => {
const app = await loadApp(topic.split(':')[1]!);

// TODO: disconnect
if (!app) {
return;
}

const existingProcess = processMetadata.get(app.externalId);

ws.send(
JSON.stringify([
topic,
'preview:status',
existingProcess
? { status: 'running', url: `http://localhost:${existingProcess.port}/` }
: { url: null, status: 'stopped', stoppedSuccessfully: true, logs: null },
]),
);
});
.on('file:updated', FileUpdatedPayloadSchema, onFileUpdated);
}
3 changes: 1 addition & 2 deletions packages/shared/src/schemas/websockets.mts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,8 +168,7 @@ export const PreviewStatusPayloadSchema = z.union([
z.object({
url: z.string().nullable(),
status: z.literal('stopped'),
stoppedSuccessfully: z.boolean(),
logs: z.string().nullable(),
code: z.number().int().nullable(),
}),
]);

Expand Down
4 changes: 2 additions & 2 deletions packages/web/src/components/apps/use-logs.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { DepsInstallLogPayloadType, PreviewLogPayloadType } from '@srcbook/share

export type LogMessage = {
type: 'stderr' | 'stdout' | 'info';
source: 'vite' | 'npm install';
source: 'srcbook' | 'vite' | 'npm';
timestamp: Date;
message: string;
};
Expand Down Expand Up @@ -78,7 +78,7 @@ export function LogsProvider({ channel, children }: ProviderPropsType) {

function onDepsInstallLog(payload: DepsInstallLogPayloadType) {
for (const row of payload.log.data.split('\n')) {
addLog(payload.log.type, 'npm install', row);
addLog(payload.log.type, 'npm', row);
}
}
channel.on('deps:install:log', onDepsInstallLog);
Expand Down
4 changes: 2 additions & 2 deletions packages/web/src/components/apps/use-package-json.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ export function PackageJsonProvider({ channel, children }: ProviderPropsType) {
async (packages?: Array<string>) => {
addLog(
'info',
'npm install',
'npm',
`Running ${!packages ? 'npm install' : `npm install ${packages.join(' ')}`}`,
);

Expand All @@ -78,7 +78,7 @@ export function PackageJsonProvider({ channel, children }: ProviderPropsType) {

addLog(
'info',
'npm install',
'npm',
`${!packages ? 'npm install' : `npm install ${packages.join(' ')}`} exited with status code ${code}`,
);

Expand Down
26 changes: 15 additions & 11 deletions packages/web/src/components/apps/use-preview.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ export interface PreviewContextValue {
status: PreviewStatusType;
stop: () => void;
start: () => void;
lastStoppedError: string | null;
exitCode: number | null;
}

const PreviewContext = createContext<PreviewContextValue | undefined>(undefined);
Expand All @@ -26,7 +26,7 @@ type ProviderPropsType = {
export function PreviewProvider({ channel, children }: ProviderPropsType) {
const [url, setUrl] = useState<string | null>(null);
const [status, setStatus] = useState<PreviewStatusType>('connecting');
const [lastStoppedError, setLastStoppedError] = useState<string | null>(null);
const [exitCode, setExitCode] = useState<number | null>(null);

const { npmInstall, nodeModulesExists } = usePackageJson();
const { addLog } = useLogs();
Expand All @@ -36,25 +36,29 @@ export function PreviewProvider({ channel, children }: ProviderPropsType) {
setUrl(payload.url);
setStatus(payload.status);

if (payload.status === 'stopped' && !payload.stoppedSuccessfully) {
// FIXME: add log here
// addLog('info', 'npm install', `Running vite ${payload.XXX}`);
setLastStoppedError(payload.logs ?? '');
} else {
setLastStoppedError(null);
switch (payload.status) {
case 'booting':
addLog('info', 'srcbook', 'Dev server is booting...');
break;
case 'running':
addLog('info', 'srcbook', `Dev server is running at ${payload.url}`);
break;
case 'stopped':
addLog('info', 'srcbook', `Dev server exited with status ${payload.code}`);
setExitCode(payload.code);
break;
}
}

channel.on('preview:status', onStatusUpdate);

return () => channel.off('preview:status', onStatusUpdate);
}, [channel, setUrl, setStatus]);
}, [channel, addLog]);

async function start() {
if (nodeModulesExists === false) {
await npmInstall();
}
addLog('info', 'npm install', `Running vite`);
channel.push('preview:start', {});
}

Expand All @@ -76,7 +80,7 @@ export function PreviewProvider({ channel, children }: ProviderPropsType) {
});

return (
<PreviewContext.Provider value={{ url, status, stop, start, lastStoppedError }}>
<PreviewContext.Provider value={{ url, status, stop, start, exitCode }}>
{children}
</PreviewContext.Provider>
);
Expand Down
8 changes: 4 additions & 4 deletions packages/web/src/routes/apps/preview.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ export default function AppPreview() {
}

function Preview() {
const { url, status, start, lastStoppedError } = usePreview();
const { url, status, start, exitCode } = usePreview();
const { nodeModulesExists } = usePackageJson();
const { togglePane } = useLogs();

Expand Down Expand Up @@ -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

<span className="text-tertiary-foreground">Dev server is stopped.</span>
) : (
<div className="flex flex-col gap-6 items-center border border-border p-8 border-dashed rounded-md">
<span className="text-red-400">Preview server stopped with an error!</span>
<span className="text-red-400">Dev server exited with an error.</span>
<Button variant="secondary" onClick={togglePane}>
Open errors pane
</Button>
Expand Down