Enable running the VS Code extension host on the workspace (Node.js) by default#3093
Enable running the VS Code extension host on the workspace (Node.js) by default#3093joao-boechat wants to merge 27 commits intomainfrom
Conversation
|
Since web workers was touched, I just wanted to double-check that the worker logic responsible for state-viz calculation on the circuit editor was still functional. I tested it and it seems to be working fine, but is there any reason to be concerned about that? |
|
@ScottCarda-MS I don't see any of the worker code I touched being referenced there. I think that the worker code in state-viz is named that way because it is supposed to be agnostic (browser and worker), so it avoids using API that's only available in browser. But my modifications shouldn't affect that part! |
There was a problem hiding this comment.
Pull request overview
This PR switches the Q# VS Code extension to run its extension host primarily on the workspace side (Node.js) by default (while still supporting VS Code for Web), and refactors the qsharp-lang npm package to use a single, platform-agnostic entry point with a unified worker strategy.
Changes:
- Build and package the VS Code extension as separate browser and node bundles (plus node worker bundles), routing worker script paths via a
__PLATFORM__build-time constant. - Unify
qsharp-lang’s browser/node entry points intomain.ts, remove the separate Node.js wasm-bindgen target, and useweb-workerto abstract Worker differences. - Update npm tests and tooling to use the unified async APIs and explicit worker paths.
Reviewed changes
Copilot reviewed 36 out of 37 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| source/vscode/src/webviewPanel.ts | Uses platform-specific output path for compiler worker script. |
| source/vscode/src/telemetry.ts | Adds platform-aware user agent / “browser release” reporting for Node vs browser. |
| source/vscode/src/qirGeneration.ts | Uses platform-specific output path for compiler worker script. |
| source/vscode/src/extension.ts | Logs platform/UI kind/remote name during activation for diagnostics. |
| source/vscode/src/documentation.ts | Uses platform-specific output path for compiler worker script. |
| source/vscode/src/debugger/debug-service-worker.ts | Simplifies worker entry to side-effect import of qsharp-lang worker module. |
| source/vscode/src/debugger/activate.ts | Uses platform-specific output path for debug worker script. |
| source/vscode/src/compilerWorker.ts | Simplifies worker entry to side-effect import of qsharp-lang worker module. |
| source/vscode/src/common.ts | Introduces __PLATFORM__ and getPlatformEnv() for consistent path selection. |
| source/vscode/src/circuit.ts | Uses platform-specific output path for compiler worker script. |
| source/vscode/package.json | Sets desktop main to node bundle, web browser to browser bundle; adds extensionKind. |
| source/vscode/build.mjs | Builds browser/node/node-worker bundles; copies wasm and external node deps (web-worker). |
| source/npm/qsharp/test/languageService.js | Updates tests to preload wasm and await async service getters. |
| source/npm/qsharp/test/diagnostics.js | Updates tests for async compiler/loader getters and explicit worker paths. |
| source/npm/qsharp/test/circuits.js | Updates tests to preload wasm and await async compiler getter. |
| source/npm/qsharp/test/basics.js | Updates tests for async APIs and explicit worker paths for worker-based services. |
| source/npm/qsharp/src/workers/worker.ts | Keeps worker-side service initialization; removes proxy-creation from this module. |
| source/npm/qsharp/src/workers/node.ts | Removes legacy node-specific worker/proxy implementation. |
| source/npm/qsharp/src/workers/main.ts | Adds unified proxy creation using web-worker for both Node and browser. |
| source/npm/qsharp/src/main.ts | Unifies browser/node entrypoint; adds wasm loading/instantiation flow and async APIs. |
| source/npm/qsharp/src/log.ts | Updates description to reflect shared browser/Node logging infrastructure. |
| source/npm/qsharp/src/language-service/worker.ts | Switches to unified worker creation module; adds implicit message listener. |
| source/npm/qsharp/src/language-service/worker-node.ts | Removes legacy node worker entrypoint. |
| source/npm/qsharp/src/language-service/language-service.ts | Updates types/imports to align with unified entrypoint structure. |
| source/npm/qsharp/src/debug-service/worker.ts | Switches to unified worker creation module; adds implicit message listener. |
| source/npm/qsharp/src/debug-service/worker-node.ts | Removes legacy node worker entrypoint. |
| source/npm/qsharp/src/debug-service/debug-service.ts | Updates types/imports to align with unified entrypoint structure. |
| source/npm/qsharp/src/compiler/worker.ts | Switches to unified worker creation module; adds implicit message listener. |
| source/npm/qsharp/src/compiler/worker-node.ts | Removes legacy node worker entrypoint. |
| source/npm/qsharp/src/compiler/compiler.ts | Removes outdated comment about node-specific wasm types. |
| source/npm/qsharp/src/common-exports.ts | Adds shared re-export surface for unified entrypoint. |
| source/npm/qsharp/src/browser.ts | Removes legacy browser-only entrypoint. |
| source/npm/qsharp/README.md | Updates docs to describe single entry point and bundling expectations. |
| source/npm/qsharp/package.json | Simplifies exports to a single entry point; adds web-worker dependency. |
| source/npm/qsharp/generate_docs.js | Updates docs generation to initialize via web wasm glue. |
| package-lock.json | Adds web-worker dependency lock entries. |
| build.py | Removes nodejs wasm-bindgen target; builds/copies only web target artifacts. |
Comments suppressed due to low confidence (3)
source/npm/qsharp/src/compiler/worker.ts:9
messageHandleris exported for backwards compatibility, but this module also unconditionally registers it viaaddEventListener. Any existing consumers that still doself.onmessage = messageHandlerwill now process each message twice (confirmed insource/playground/src/compiler-worker.ts). Consider removing the implicit registration, or making registration idempotent / updating consumers so only one handler is attached.
source/npm/qsharp/src/language-service/worker.ts:9- Same issue as the compiler worker: exporting
messageHandlerwhile also registering it withaddEventListenercan lead to double-handling when older consumers assignself.onmessage = messageHandler(confirmed insource/playground/src/language-service-worker.ts). Prefer a single registration mechanism to preserve backwards compatibility.
source/npm/qsharp/src/debug-service/worker.ts:9 messageHandleris exported for backwards compatibility, but this module also unconditionally registers it withaddEventListener, which can lead to double-handling if any consumer also assignsself.onmessage = messageHandler. If backwards compatibility is required, prefer a single/explicit registration path or make registration idempotent.
source/vscode/build.mjs
Outdated
| * Copy external node dependencies into node_modules/ under the extension | ||
| * directory so they can be resolved at runtime (e.g. when installed as a VSIX). | ||
| */ | ||
| function copyNodeExternals() { |
There was a problem hiding this comment.
This is a bit weird. If web-worker is a dependency we should just declare it as a dependency in package.json so it can be properly installed into node_modules by the package manager.
That said, I don't know why we can't bundle web-worker for node, the same way we do for the browser?
There was a problem hiding this comment.
We can't bundle it because it would break the functionality of web-worker. The package doesn't really support bundling for node, and we have to treat it as external.
The reason I am copying the web-worker to the local directory is because we are using workspaces and automatically npm will hoist node_modules to the root directory. But I've changed it and now, instead of doing this procedure in the build script, I use postInstall to achieve the same goal in a simpler way. I also declared it as a dependency for our vscode extension and an optional dependency for npm/qsharp (as it is only needed for the node case).
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Mine Starks <16928427+minestarks@users.noreply.github.com>
There was a problem hiding this comment.
Now in this folder, we ended up with:
workers
|- common.ts
|- main.ts
|- worker.ts
this makes me think main.ts runs main-thread side, worker.ts runs worker-thread side. So far this makes sense, but then I'd expect common.ts to run in both.... which isn't true. "common" used to mean "common to browser and node" but now, with your changes, the meaning has shifted to "common to main and worker".
Would it be a pain to move stuff out of common.ts into the files they belong?
I'm copy-pasting how Copilot broke it down, in case it's helpful.
Your intuition is right — the separation is off. Here's what each file actually does and where the problem lies:
main.ts (~85 lines) — "main thread" side. Exports createProxy(), which spins up a Worker, sends it the WASM module, and returns a proxy object. It imports createProxyInternal from common.
worker.ts (~60 lines) — "worker thread" side. Exports createWorker(), which sets up the onmessage handler inside the Web Worker, initializes WASM, and wires up the service. It imports initService from common.
common.ts (~575 lines) — this is where the naming breaks down. It contains:
| What | Lines | Actually used by |
|---|---|---|
Shared type definitions (ServiceProtocol, MethodMap, RequestMessage, ResponseMessage, etc.) |
~100 | Both |
serializeIfError / deserializeIfError |
~50 | One each side |
createProxyInternal() — proxy queue, event dispatch, state management |
~150 | main.ts only |
createDispatcher() + initService() — service wrapping, logging setup |
~130 | worker.ts only |
So roughly 65% of "common" is not common at all. createProxyInternal is purely main-thread logic, and createDispatcher/initService are purely worker-thread logic. The file is really "everything that isn't the thin entrypoint wrapper."
A cleaner split would be:
types.ts— shared types and message definitions, plus error serialization helpers (the actually common stuff)- main.ts — absorbs
createProxyInternalalongsidecreateProxy - worker.ts — absorbs
createDispatcherandinitServicealongsidecreateWorker
This way each file owns its full logic rather than having the real implementation buried in a misleadingly-named "common" file. Want me to do the refactor?
| @@ -402,6 +403,9 @@ export function sendTelemetryEvent<E extends keyof EventTypes>( | |||
| } | |||
|
|
|||
| function getBrowserRelease(): string { | |||
There was a problem hiding this comment.
I can't tell if I'm just being dense, but.... is getBrowserRelease or getUserAgent actually being used by any of the telemetry code? The only consumer I see is in networkRequests. If that's the case, maybe we should just move these helpers to that file.
My initial concern was whether this would impact our telemetry. (We use telemetry data to classify usage by OS, etc)
| const compilerWorkerScriptPath = Uri.joinPath( | ||
| extensionUri, | ||
| "./out/compilerWorker.js", | ||
| `./out/${getPlatformEnv()}/compilerWorker.js`, |
There was a problem hiding this comment.
can this import and use loadCompilerWorker from common.ts?
| const compilerWorkerScriptPath = Uri.joinPath( | ||
| extensionUri, | ||
| "./out/compilerWorker.js", | ||
| `./out/${getPlatformEnv()}/compilerWorker.js`, |
There was a problem hiding this comment.
Can this use loadCompilerWorker from common.ts too?
| // Licensed under the MIT License. | ||
|
|
||
| // Logging infrastructure for JavaScript environments (e.g. browser and node.js) | ||
| // Logging infrastructure shared across browser and Node.js environments |
There was a problem hiding this comment.
What was wrong with the old comment??
Summary
This PR makes the Q# VS Code extension run its extension host primarily on the workspace side (Node.js) rather than on the UI side (browser). This is important for remote development scenarios — WSL, SSH, Codespaces, Dev Containers — where keeping the extension host close to the workspace avoids bugs and simplifies the architecture. The extension still supports running on the UI side for VS Code for Web.
To make this work, the Node.js entry point of the
qsharp-langnpm package needed to function correctly. Rather than fixing the separate Node.js codepath, we unified both entry points into a single platform-agnostic module, using theweb-workerpackage to abstract away Worker API differences between browsers and Node.js.Motivation
The Q# extension was originally built as a web-first extension, meaning the extension host always ran on the UI side (browser), even in desktop VS Code. This caused issues in remote development scenarios (WSL, SSH, Codespaces, Dev Containers) where the extension host needs to run close to the workspace for correct behavior. By making the Node.js entry point the default, the extension host runs on the workspace side, avoiding these issues.
Additionally, the npm package previously maintained two parallel codepaths —
browser.tsandmain.ts— each with its own wasm loading strategy, worker proxy implementation, and per-service worker scripts. The build system also produced two separate wasm-bindgen targets (webandnodejs) and used npm conditional exports to route consumers to the right entry point. The Node.js path was incomplete and buggy, and the duplication made changes error-prone. Unifying these into a single platform-agnostic entry point simplifies the codebase and eliminates this class of bugs.Architecture (of qsharp-lang)
Before
graph TD subgraph "npm package entry points" B["browser.ts (browser entry)"] M["main.ts (Node.js entry)"] end B --> WB["workers/browser.ts"] M --> WN["workers/node.ts"] WB --> CWB["compiler/worker-browser.ts"] WB --> LSWB["language-service/worker-browser.ts"] WB --> DSWB["debug-service/worker-browser.ts"] WN --> CWN["compiler/worker-node.ts"] WN --> LSWN["language-service/worker-node.ts"] WN --> DSWN["debug-service/worker-node.ts"] CWB --> WASMW["lib/web/qsc_wasm.js"] CWN --> WASMN["lib/nodejs/qsc_wasm.cjs"]After
graph TD subgraph "npm package entry point" M["main.ts (single entry)"] end M --> WM["workers/main.ts (proxy, uses web-worker pkg)"] WW["workers/worker.ts (runs inside worker)"] WW --> CW["compiler/worker.ts"] WW --> LSW["language-service/worker.ts"] WW --> DSW["debug-service/worker.ts"] CW --> WASM["lib/web/qsc_wasm.js"] LSW --> WASM DSW --> WASM WM -.->|spawns| CW WM -.->|spawns| LSW WM -.->|spawns| DSWChanges
qsharp-langnpm package (source/npm/qsharp/)browser.tsand merged all functionality intomain.ts— a single platform-agnostic entry point.worker-browser.ts/worker-node.ts) into a singleworker.tseach.workers/browser.tsandworkers/node.tswithworkers/worker.ts(worker-side) andworkers/main.ts(main-thread proxy using theweb-workerpackage).common-exports.tsfor shared re-exports.webwasm-bindgen target is used; thenodejstarget is no longer needed.package.jsonexports — removed conditionalbrowser/node/defaultrouting.web-worker(^1.5.0) as a dependency.async(getCompiler(),getDebugService(),getLanguageService(),getProjectLoader()), and worker factory functions now require an explicit worker path argument.VS Code extension (
source/vscode/)__PLATFORM__build-time constant ("browser"or"node") to resolve the correct worker script paths at runtime.extension.ts) and the worker entry points (compilerWorker.ts,debug-service-worker.ts).wasm/), which is the main reason the overall VSIX size only grew from 3.9 MB to 4.09 MB despite adding a full Node.js bundle.web-workerpackage must be marked as an external dependency in the Node.js esbuild bundle. This is becauseweb-workerperforms an internal runtime check that breaks if its code is inlined into the bundle. As a result, it is copied into the extension'snode_modules/so it can be resolved at runtime (e.g. when installed from a VSIX)../out/node/extension.js.Platform,UI Kind, andRemotename during activation for diagnostics.Build system (
build.py)webwasm-bindgen target is built. Removed thenodejstarget and its.js→.cjsrenaming logic.Tests (
source/npm/qsharp/test/)loadWasmModule, async service getters, explicit worker paths).Extra