Support loading libraries into an already-running language server session - #63
Open
SVAGEN26 wants to merge 6 commits into
Open
Support loading libraries into an already-running language server session#63SVAGEN26 wants to merge 6 commits into
SVAGEN26 wants to merge 6 commits into
Conversation
…sion Previously the only way to pick up a new library was a full server restart with the library baked into initializationOptions/workspace folders, since loadLibrary was only ever called from ModelicaServer.initialize. This wires up onDidChangeWorkspaceFolders and onDidChangeConfiguration to load newly announced libraries at runtime, tracking already-loaded paths so repeated notifications are no-ops. Unloading a removed workspace folder is out of scope for now and still requires a restart. Fixes OpenModelica#62. Co-Authored-By: JKRT <jtinnerholm@gmail.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CI's "Build package" step runs the root esbuild.config.js, which bundles server.ts into ./out/server.js. server/out/server.js is a different build (server's own esbuild.config.js, used for the standalone SEA binary) that CI's main test job never produces. Co-Authored-By: JKRT <jtinnerholm@gmail.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
JKRT
reviewed
Jul 9, 2026
| * Loads libraries added to the workspace after startup, so a client can | ||
| * make a new library available (e.g. to resolve an external `within` | ||
| * reference) without restarting the server. Folders can only be added; | ||
| * there is currently no support for unloading a removed folder. |
Member
There was a problem hiding this comment.
For long OMEdit sessions this too would be neat to have
Contributor
Author
There was a problem hiding this comment.
This is a Claude Code agent acting on behalf of @JKRT.
Done in 48d0937. onDidChangeWorkspaceFolders now handles event.removed: ModelicaProject.removeLibrariesUnder drops every library at or below the removed root (and its documents), Analyzer.unloadLibrary exposes it, and the server forgets the path from #loadedLibraryPaths so the same folder can be re-added later. Covered by unit tests plus an end-to-end add -> resolves -> remove -> stops resolving -> re-add -> resolves cycle.
connection.workspace.onDidChangeWorkspaceFolders throws synchronously if the client didn't advertise workspace.workspaceFolders. Since register() runs uncaught inside connection.onInitialize, that turned into a JSON-RPC error response for the whole initialize request, silently breaking every LSP feature (hover, definition, symbols) for that session. Confirmed by probing the built bundle directly with and without the capability present. Co-Authored-By: JKRT <jtinnerholm@gmail.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Subscribing via connection.workspace.onDidChangeWorkspaceFolders during the onInitialize handler happens before the server capabilities are filled, so the vscode-languageserver library treats the (statically declared) workspace folder change notification as not-yet-registered and fires a dynamic client/registerCapability request. The real vscode-languageclient rejects that redundant registration, and the resulting unhandled promise rejection crashes the server process right after initialize. Every subsequent request then goes unanswered, which is why all four e2e client tests (hover, documentSymbol, declaration) timed out. Move the subscription to onInitialized, which runs after capabilities are filled: the notification is then already auto-registered, so no dynamic registration is sent and nothing can be rejected. Verified by driving the built bundle over stdio with a client that rejects the registration (crashes before the fix, survives after). Add a regression test that rejects the workspace folder registration the way vscode-languageclient does and asserts the server process stays alive and keeps answering requests. Co-Authored-By: JKRT <jtinnerholm@gmail.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The three new end-to-end tests exercise paths the first test did not:
- loading a library pushed via workspace/didChangeConfiguration
- re-announcing an already-loaded library (dedup no-op via the
"already loaded" log) without disturbing resolution
- announcing a nonexistent folder alongside two real libraries where
one (RuntimeLoadLibC.P) extends another (RuntimeLoadLibA.M), proving
the bad entry neither crashes the server nor blocks cross-library
resolution
Writing them surfaced a real bug: the workspace folder subscription was
set up after `await connection.client.register(didChangeWatchedFiles)`
in onInitialized, so a didChangeWorkspaceFolders notification arriving
before that await resolved was delivered with no handler attached and
silently dropped. The original test only passed because an intermediate
request gave the registration time to complete. Subscribe synchronously
at the top of onInitialized instead (capabilities are already filled, so
no premature dynamic registration is sent).
Co-Authored-By: JKRT <jtinnerholm@gmail.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Addresses review feedback on OpenModelica#63: for long-running sessions (e.g. OMEdit) a user may unload a library, so the server should free it at runtime instead of only supporting additions. - ModelicaProject.removeLibrariesUnder(rootPath) drops every library at or below a removed root, along with its documents. - Analyzer.unloadLibrary(uri) exposes that for a workspace/library root. - onDidChangeWorkspaceFolders now handles event.removed: it unloads the affected libraries and forgets their paths from #loadedLibraryPaths so the same folder can be re-added later. A removed folder with a non-file URI is ignored rather than crashing the handler. Tests: Analyzer.unloadLibrary unit tests (unload + report once, nested under a removed root, no-match returns empty) and an end-to-end add -> resolves -> remove -> stops resolving -> re-add -> resolves cycle. Co-Authored-By: JKRT <jtinnerholm@gmail.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Member
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a Claude Code agent acting on behalf of @JKRT.
Summary
Closes #62.
Previously the only way for the server to pick up a new library was a full restart:
Analyzer.loadLibrarywas only ever invoked fromModelicaServer.initialize, so there was no code path to add a library once the server was running. This blocked OMEdit's go-to-definition from cleanly resolvingwithinreferences into libraries that were not loaded yet (see the workaround discussion in OpenModelica/OpenModelica#15925).ModelicaServernow tracks the resolved paths of libraries/workspaces it has already loaded (#loadedLibraryPaths), so re-announcing the same folder is a no-op instead of loading it (and its documents) a second time.workspace/didChangeWorkspaceFoldersis now handled: folders added after startup are loaded into the running analyzer the same way initial workspace folders are. Removed folders are not unloaded yet (out of scope here) — a restart is still needed to fully drop a library.workspace/didChangeConfigurationis now handled: a client can push an updatedmodelica.librarieslist and any new entries get loaded without a restart.initialize()into#tryLoadLibrary/#loadConfiguredLibraryso the new runtime paths reuse it instead of re-implementing it.Verification
Added
server/src/test/runtimeLibraryLoad.test.ts, an end-to-end test that spawns the builtout/server.jsbundle over its real stdio JSON-RPC transport (same transport an editor uses) and:RuntimeLoadLibBloaded (fixtures underserver/src/test/fixtures/), whereRuntimeLoadLibB.Ndoesextends RuntimeLoadLibA.M.textDocument/definitiononRuntimeLoadLibA.Mreturns empty — the reference is unresolved becauseRuntimeLoadLibAisn't loaded.workspace/didChangeWorkspaceFoldersannouncingRuntimeLoadLibA, without restarting the process.textDocument/definitionnow resolves.This reproduces (and now guards against regressing) the exact gap reported in #62 — I verified the old behavior against this same test harness before making the fix, and it failed at step 4 as expected.
Test plan
npm run test:server— 28 passing, including the new end-to-end test.npm run lint— clean.npm run esbuild+ manual typecheck (tsc --noEmit) — clean.