From 4c7c4fc7814eb286b8292de752dd005f85d836f3 Mon Sep 17 00:00:00 2001 From: Ben Reinhart Date: Sun, 14 Jul 2024 13:20:05 -0700 Subject: [PATCH] Use tsserver geterr command & events. reloadProjects for file renames (#132) --- packages/api/server/ws.mts | 114 +++++++++++++++++------------ packages/api/srcbook/path.mts | 4 + packages/api/tsserver/tsserver.mts | 109 ++++++++++++++++++++------- packages/api/tsserver/utils.mts | 2 +- 4 files changed, 155 insertions(+), 74 deletions(-) diff --git a/packages/api/server/ws.mts b/packages/api/server/ws.mts index f88d17bc..2b7c615e 100644 --- a/packages/api/server/ws.mts +++ b/packages/api/server/ws.mts @@ -43,7 +43,7 @@ import { import tsservers from '../tsservers.mjs'; import { TsServer } from '../tsserver/tsserver.mjs'; import WebSocketServer from './ws-client.mjs'; -import { pathToCodeFile } from '../srcbook/path.mjs'; +import { filenameFromPath, pathToCodeFile } from '../srcbook/path.mjs'; import { normalizeDiagnostic } from '../tsserver/utils.mjs'; import { removeCodeCellFromDisk } from '../srcbook/index.mjs'; @@ -312,12 +312,29 @@ async function cellUpdate(payload: CellUpdatePayloadType) { tsserver.close({ file: oldFilePath }); tsserver.open({ file: newFilePath, fileContent: cellAfterUpdate.source }); - // Send all diagnostics so that other cells that import this updated cell - // are informed about any updates (changes to exports, file renames). - // - // TODO: Can we be smarter and only do this for cells that import - // this cell, rather than all cells? - sendAllTypeScriptDiagnostics(tsserver, session); + // TODO: Given the amount of differences here and elsewhere when renaming cells, + // it's probably worth it at this point to make those separate websocket events. + if (oldFilePath !== newFilePath) { + // Tsserver can get into a bad state if we don't reload the project after renaming a file. + // This consistently happens under the following condition: + // + // 1. Rename a `a.ts` that is imported by `b.ts` to `c.ts` + // 2. Semantic diagnostics report an error in `b.ts` that `a.ts` doesn't exist + // 3. Great, all works so far. + // 4. Rename `c.ts` back to `a.ts`. + // 5. Semantic diagnostics still report an error in `b.ts` that `a.ts` doesn't exist. + // 6. This is wrong, `a.ts` does exist. + // + // If we reload the project, this issue resolves itself. + // + // NOTE: reloading the project sends diagnostic events without calling `geterr`. + // However, it seems to take a while for the diagnostics to be sent, so we still + // request it below. + // + tsserver.reloadProjects(); + } + + requestAllDiagnostics(tsserver, session); } } @@ -351,44 +368,60 @@ async function cellDelete(payload: CellDeletePayloadType) { const file = pathToCodeFile(updatedSession.dir, cell.filename); const tsserver = tsservers.get(updatedSession.id); tsserver.close({ file }); - sendAllTypeScriptDiagnostics(tsserver, updatedSession); + requestAllDiagnostics(tsserver, updatedSession); } } } /** - * Send semantic diagnostics for a TypeScript cell to the client. + * Request async diagnostics for all files in the project. */ -async function sendTypeScriptDiagnostics( - tsserver: TsServer, - session: SessionType, - cell: CodeCellType, -) { - const response = await tsserver.semanticDiagnosticsSync({ - file: pathToCodeFile(session.dir, cell.filename), - }); +function requestAllDiagnostics(tsserver: TsServer, session: SessionType, delay = 0) { + const codeCells = session.cells.filter((cell) => cell.type === 'code') as CodeCellType[]; + const files = codeCells.map((cell) => pathToCodeFile(session.dir, cell.filename)); + tsserver.geterr({ files, delay }); +} - if (!response.success) { - console.warn(`Failed to get diagnostics for cell ${cell.id}: ${response.message}`); - return; - } +function createTsServer(session: SessionType) { + const tsserver = tsservers.create(session.id, { cwd: session.dir }); + + const sessionId = session.id; + + tsserver.onSemanticDiag(async (event) => { + const eventBody = event.body; + + // Get most recent session state + const session = await findSession(sessionId); + + if (!eventBody || !session) { + return; + } - // The client will always reset diagnostics when the server sends them. - // Therefore, it is important to send diagnostics even when the list is - // empty because the client will not clear stale diagnostics otherwise. - const diagnostics = response.body || []; - wss.broadcast(`session:${session.id}`, 'tsserver:cell:diagnostics', { - cellId: cell.id, - diagnostics: diagnostics.map(normalizeDiagnostic), + const filename = filenameFromPath(eventBody.file); + const cells = session.cells.filter((cell) => cell.type === 'code') as CodeCellType[]; + const cell = cells.find((c) => c.filename === filename); + + if (!cell) { + return; + } + + wss.broadcast(`session:${session.id}`, 'tsserver:cell:diagnostics', { + cellId: cell.id, + diagnostics: eventBody.diagnostics.map(normalizeDiagnostic), + }); }); -} -function sendAllTypeScriptDiagnostics(tsserver: TsServer, session: SessionType) { + // Open all code cells in tsserver for (const cell of session.cells) { if (cell.type === 'code') { - sendTypeScriptDiagnostics(tsserver, session, cell); + tsserver.open({ + file: pathToCodeFile(session.dir, cell.filename), + fileContent: cell.source, + }); } } + + return tsserver; } async function tsserverStart(payload: TsServerStartPayloadType) { @@ -402,21 +435,10 @@ async function tsserverStart(payload: TsServerStartPayloadType) { throw new Error(`tsserver can only be used with TypeScript Srcbooks.`); } - if (!tsservers.has(session.id)) { - const tsserver = tsservers.create(session.id, { cwd: session.dir }); - - // Open all code cells in tsserver - for (const cell of session.cells) { - if (cell.type === 'code') { - tsserver.open({ - file: pathToCodeFile(session.dir, cell.filename), - fileContent: cell.source, - }); - } - } - } - - sendAllTypeScriptDiagnostics(tsservers.get(session.id), session); + requestAllDiagnostics( + tsservers.has(session.id) ? tsservers.get(session.id) : createTsServer(session), + session, + ); } async function tsserverStop(payload: TsServerStopPayloadType) { diff --git a/packages/api/srcbook/path.mts b/packages/api/srcbook/path.mts index 30741de7..2a949014 100644 --- a/packages/api/srcbook/path.mts +++ b/packages/api/srcbook/path.mts @@ -20,3 +20,7 @@ export function pathToTsconfigJson(baseDir: string) { export function pathToCodeFile(baseDir: string, filename: string) { return Path.join(baseDir, 'src', filename); } + +export function filenameFromPath(filePath: string) { + return Path.basename(filePath); +} diff --git a/packages/api/tsserver/tsserver.mts b/packages/api/tsserver/tsserver.mts index 13aef28d..5f4512b1 100644 --- a/packages/api/tsserver/tsserver.mts +++ b/packages/api/tsserver/tsserver.mts @@ -1,14 +1,44 @@ -import { parseTsServerMessages } from './utils.mjs'; -import type { ChildProcess } from 'child_process'; +import EventEmitter from 'node:events'; +import type { ChildProcess } from 'node:child_process'; import type { server as tsserver } from 'typescript'; +import { parseTsServerMessages } from './utils.mjs'; -export class TsServer { +/** + * This class provides a wrapper around a process running tsserver and is used to communicate + * with the server, mainly to support diagnostics for user code (type errors, sytnax errors, + * type definitions, etc). + * + * tsserver is not documented. Here is a brief overview. + * + * tsserver is a process which listens for messages over stdin and + * sends messages over stdout. tsserver has three types of messages: + * + * 1. Request: A request from the client to the server. + * 2. Response: A response from the server to a specific client request. + * 3. Event: An event from the server to the client. + * + * Request and responses are identified a unique number called `seq`. `seq` is incremented + * for each request the client sends. The client will send a `seq` field with its request + * and the server will provide a `request_seq` in its response which is used to tie a message + * from the server to a specific request from the client. + * + * Events can arrive at any time but are often used as an asynchronous response from the server. + * For example, syntax and semantic diagnostics are sent as events when using the `geterr` command. + * + * Most of this is learned by reading through the source (protocol.ts) as well as trial + * and error. They also have an introduction, but it's hardly useful. See links below. + * + * - https://github.com/microsoft/TypeScript/blob/v5.5.3/src/server/protocol.ts + * - https://github.com/microsoft/TypeScript/wiki/Standalone-Server-(tsserver) + */ +export class TsServer extends EventEmitter { private _seq: number = 0; private buffered: Buffer = Buffer.from(''); private readonly process: ChildProcess; private readonly resolvers: Record void> = {}; constructor(process: ChildProcess) { + super(); this.process = process; this.process.stdout?.on('data', (chunk) => { const { messages, buffered } = parseTsServerMessages(chunk, this.buffered); @@ -32,7 +62,7 @@ export class TsServer { if (!resolve) { console.warn( - `Received a response for command '${response.command}' and request_seq '${response.request_seq}' but no resolver was found. This may be a bug in the code.`, + `Received a response for command '${response.command}' and request_seq '${response.request_seq}' but no resolver was found. This may be a bug in the code.\n\nResponse:\n${JSON.stringify(response, null, 2)}\n`, ); return; @@ -43,8 +73,8 @@ export class TsServer { resolve(response); } - private handleEvent(_event: tsserver.protocol.Event) { - // Ignoring telemetry events for now + private handleEvent(event: tsserver.protocol.Event) { + this.emit(event.event, event); } private send(request: tsserver.protocol.Request) { @@ -58,10 +88,32 @@ export class TsServer { }); } + /** + * Wrapper around the `semanticDiag` event for convenience and type safety. + */ + onSemanticDiag(callback: (event: tsserver.protocol.DiagnosticEvent) => void) { + this.on('semanticDiag', callback); + } + + /** + * Wrapper around the `syntaxDiag` event for convenience and type safety. + */ + onSyntaxDiag(callback: (event: tsserver.protocol.DiagnosticEvent) => void) { + this.on('syntaxDiag', callback); + } + + /** + * Wrapper around the `suggestionDiag` event for convenience and type safety. + */ + onSuggestionDiag(callback: (event: tsserver.protocol.DiagnosticEvent) => void) { + this.on('suggestionDiag', callback); + } + /** * Shutdown the underlying tsserver process. */ shutdown() { + this.removeAllListeners(); return this.process.kill('SIGTERM'); } @@ -94,57 +146,60 @@ export class TsServer { } /** - * Get info about the project. + * Ask tsserver to send diagnostics for a set of files. * - * This can be useful during development to inspect the tsserver integration. + * This is used to get the errors for a set of files in a project. + * + * Note that the diagnostics are sent as asynchronous events instead of responding to this request. */ - projectInfo(args: tsserver.protocol.ProjectInfoRequestArgs) { - return this.sendWithResponsePromise({ + geterr(args: tsserver.protocol.GeterrRequestArgs) { + this.send({ seq: this.seq, type: 'request', - command: 'projectInfo', + command: 'geterr', arguments: args, }); } /** - * Get info about a term at a specific location in a file. + * Reload the project in tsserver. * - * This is used for type definitions and documentation lookups on hover. + * This is used to tell tsserver to reload the project configuration + * which helps ensure that the project is up-to-date. This helps resolve + * errors that can occur when renaming files. */ - quickinfo(args: tsserver.protocol.FileLocationRequestArgs) { - return this.sendWithResponsePromise({ + reloadProjects() { + this.send({ seq: this.seq, type: 'request', - command: 'quickinfo', - arguments: args, + command: 'reloadProjects', }); } /** - * Get semantic information about a file. + * Get info about the project. * - * This is used to report type errors in a file. + * This can be useful during development to inspect the tsserver integration. */ - semanticDiagnosticsSync(args: tsserver.protocol.SemanticDiagnosticsSyncRequestArgs) { - return this.sendWithResponsePromise({ + projectInfo(args: tsserver.protocol.ProjectInfoRequestArgs) { + return this.sendWithResponsePromise({ seq: this.seq, type: 'request', - command: 'semanticDiagnosticsSync', + command: 'projectInfo', arguments: args, }); } /** - * Get syntactic information about a file. + * Get info about a term at a specific location in a file. * - * This is used to report syntax errors in a file. + * This is used for type definitions and documentation lookups on hover. */ - syntacticDiagnosticsSync(args: tsserver.protocol.SyntacticDiagnosticsSyncRequestArgs) { - return this.sendWithResponsePromise({ + quickinfo(args: tsserver.protocol.FileLocationRequestArgs) { + return this.sendWithResponsePromise({ seq: this.seq, type: 'request', - command: 'syntacticDiagnosticsSync', + command: 'quickinfo', arguments: args, }); } diff --git a/packages/api/tsserver/utils.mts b/packages/api/tsserver/utils.mts index fc27d545..e5c0bbe0 100644 --- a/packages/api/tsserver/utils.mts +++ b/packages/api/tsserver/utils.mts @@ -68,7 +68,7 @@ export function normalizeDiagnostic( }; } else { return { - // From what I can tell, code should always be present depsite the type. + // From what I can tell, code should always be present despite the type. // If it's not, we use 1000 as the 'unknown' error code, which is not a // code defined in diagnosticMessages.json in TypeScript's source. code: diagnostic.code || 1000,