fix(server): drop fs dependency (#152)#156
Conversation
There was a problem hiding this comment.
Pull request overview
This PR moves stylesheet content loading from the language server to the VS Code client so the server no longer reads from the host filesystem, and updates the manifest to advertise virtual workspace support.
Changes:
- Client discovers stylesheets and sends
{ uri, languageId, text }initialization payloads. - Server consumes stylesheet text directly instead of importing/using
fs. - Tests add a workspace-fs-based stylesheet loading path.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
client/src/extension.ts |
Reads discovered stylesheet contents via vscode.workspace.fs and passes them to the server. |
server/src/server.ts |
Builds the initial stylesheet map from client-provided stylesheet payloads. |
server/src/types.ts |
Replaces the old URI/fsPath initialization shape with a stylesheet text payload type. |
tests/src/findDefinition.test.ts |
Adds coverage for stylesheet maps built from vscode.workspace.fs.readFile. |
package.json |
Marks virtual workspaces as supported. |
Comments suppressed due to low confidence (3)
client/src/extension.ts:186
- This new use of
workspace.fsrequires a newer VS Code API than the top-level manifest currently advertises (engines.vscodeis still^1.33.0). Users on supported older VS Code versions can install the extension, but this call will fail and the server will start with an empty stylesheet map unless the engine range is bumped or a fallback is kept.
const bytes = await Workspace.fs.readFile(u);
client/src/extension.ts:190
Bufferis a Node global, but this extension also declares a browser entrypoint and the surrounding comments claim web compatibility. In the browser extension host this decode step can throw before any stylesheet is sent; use a browser-safe decoder or provide a polyfill if web support is intended.
text: Buffer.from(bytes).toString("utf8"),
client/src/extension.ts:184
- Reading every discovered stylesheet with an unbounded
Promise.allcan issue thousands of concurrent reads and keep both byte buffers and decoded strings in memory before the language client starts. Large or remote/virtual workspaces can stall activation or exhaust memory; consider bounded concurrency and/or a lazy/on-demand loading protocol.
const stylesheets: Stylesheet[] = (
await Promise.all(
file_searches.map(async (u): Promise<Stylesheet | null> => {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "virtualWorkspaces": { | ||
| "supported": false, | ||
| "description": "The extension currently relies on the `fs` module but it should be easy to change this. Please make a PR to help." | ||
| "supported": true |
There was a problem hiding this comment.
Good catch — reverted in 02868fe. The documentSelector and the scheme guard at the top of didOpenTextDocument still gate on file/untitled, so advertising virtual workspace support here was premature even though the server side is ready. Expanding the selector to cover vscode-vfs, vscode-test-web, etc. is a separate follow-up; this PR's scope is just dropping fs.
| const potentialFiles: Uri[] = file_searches.filter( | ||
| (uri: Uri) => uri.scheme === "file" | ||
| ); | ||
| ).then(async (file_searches) => { |
There was a problem hiding this comment.
Right — fixed in 02868fe. Added a pendingClientFolders: Set<string> that's claimed synchronously before any await, so concurrent didOpenTextDocument calls in the same folder see the in-flight client and bail. The continuation also bails out if the folder was removed (workspaceFolders change) while reads were in progress. Same shape as the fix in PR #155 for issue #154 — when both land they'll converge on the same pattern.
| assert.deepStrictEqual(lines, [0, 16, 16, 19]); | ||
| }); | ||
|
|
||
| test("stylesheets loaded via vscode.workspace.fs are discoverable", async () => { |
There was a problem hiding this comment.
Fair. Added a new test resolves selectors across non-file URI schemes in 02868fe that constructs a StylesheetMap with vscode-vfs:// and vscode-test-web:// URIs and verifies findDefinition resolves selectors and returns Locations on those schemes. That nails down the scheme-agnosticism contract on the server side. (The client's documentSelector still gates on file/untitled, so the full virtual-workspace flow needs separate work — see the related thread above.)
Squashed: includes the original fs-removal commit and the follow-up review fixes (TextDecoder for browser compat, bounded read concurrency, race-guard during async file reads, virtualWorkspaces.supported reverted to false until documentSelector is broadened, engines.vscode bumped to the workspace.fs minimum). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
02868fe to
8fa4abe
Compare
Builds on top of #156's pendingClientFolders race guard with three small robustness improvements identified in the #155 review: - Register the LanguageClient in `clients` BEFORE calling start(). If a workspace folder is removed mid-start, onDidChangeWorkspaceFolders can now find and stop the client. - Wrap client.start() in try/catch; on failure remove from `clients` so the folder can be retried on the next document-open. - Wrap Workspace.findFiles() in try/catch and log via telemetry instead of letting the rejection bubble up as an unhandled promise (the outer call site swallows it). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Builds on top of #156's pendingClientFolders race guard with three small robustness improvements identified in the #155 review: - Register the LanguageClient in `clients` BEFORE calling start(). If a workspace folder is removed mid-start, onDidChangeWorkspaceFolders can now find and stop the client. - Wrap client.start() in try/catch; on failure remove from `clients` so the folder can be retried on the next document-open. - Wrap Workspace.findFiles() in try/catch and log via telemetry instead of letting the rejection bubble up as an unhandled promise (the outer call site swallows it). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Builds on top of #156's pendingClientFolders race guard with three small robustness improvements identified in the #155 review: - Register the LanguageClient in `clients` BEFORE calling start(). If a workspace folder is removed mid-start, onDidChangeWorkspaceFolders can now find and stop the client. - Wrap client.start() in try/catch; on failure remove from `clients` so the folder can be retried on the next document-open. - Wrap Workspace.findFiles() in try/catch and log via telemetry instead of letting the rejection bubble up as an unhandled promise (the outer call site swallows it). Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
vscode.workspace.fs.readFile(virtual/web-workspace safe) and sends{uri, languageId, text}tuples to the server during LSP initialization.fsand never touches the host file system, so the LSP backend works in virtual workspaces and on the web build.virtualWorkspaces.supportedtotruein the extension manifest now that the prerequisite is gone.Test plan
yarn build(zero TS errors)yarn test(25 passing, including newstylesheets loaded via vscode.workspace.fs are discoverablecase intests/src/findDefinition.test.ts)yarn lintyarn prettieron changed filesCloses #152